[RESOLVED] [Code Review] Looking for advice

Setup, Configuration and Use Scripts & Rules
1

Hello everyone,

I currently collect share price data using the following rules. As you can see I’m just repeating the same rule with a small change. Is there room for improvement or maybe in a more programmable way? Nothing broken with what I have in place, I just want to make sure there isn’t a better way to do things. The API I’m using is free, so limits the number of requests per day. This means I need to limit the number of times I run the rules.

Thanks in advanced, all comments are welcome.

rule "Get TW Share Price From CSV Data"
when
    Time cron "0 15 * ? * * *"
then
    val csv = sendHttpGetRequest("https://www.alphavantage.co/query?function=TIME_SERIES_INTRADAY&symbol=LON:TW&interval=15min&outputsize=compact&datatype=csv&apikey=MYAPIKEY").split("\n")
    val firstLine = csv.get(2).split(",")
    val Price = firstLine.get(2).trim
    logInfo("org.openhab", "Share Price Update: TW; " + Price)
    TW_Current_Price.sendCommand(Price)
end

rule "Get TCG Share Price From CSV Data"
when
    Time cron "0 16 * ? * * *"
then
    val csv = sendHttpGetRequest("https://www.alphavantage.co/query?function=TIME_SERIES_INTRADAY&symbol=LON:TCG&interval=15min&outputsize=compact&datatype=csv&apikey=MYAPIKEY").split("\n")
    val firstLine = csv.get(2).split(",")
    val Price = firstLine.get(2).trim
    logInfo("org.openhab", "Share Price Update: TCG; " + Price)
    TC_Current_Price.sendCommand(Price)
end

rule "Get SLA Share Price From CSV Data"
when
    Time cron "0 17 * ? * * *"
then
    val csv = sendHttpGetRequest("https://www.alphavantage.co/query?function=TIME_SERIES_INTRADAY&symbol=LON:SLA&interval=15min&outputsize=compact&datatype=csv&apikey=MYAPIKEY").split("\n")
    val firstLine = csv.get(2).split(",")
    val Price = firstLine.get(2).trim
    logInfo("org.openhab", "Share Price Update: SLA; " + Price)
    SL_Current_Price.sendCommand(Price)
end
2

Is there one api call that gets ALL the share prices?
Then you could filter the response and get your three share prices at once

3

Sadly not. Agreed it would make things a lot easier. Although I’m no developer and do all my coding work by searching for examples, I do know that there is always room for improvements. Or at least alternative methods.

4

Does it also have a limit in number of requests per minute? Will it cause problems if we made all the requests for the prices of each stock individually all together but then spread out this barrage so you don’t exceed your limit per day?

That could greatly simplify the Rules as the sendHttpGetRequests and the cron trigger can be moved to the http.cfg file.

Then you would need just one Rule (I’m assuming that there is something in the returned CSV that will tell you what stock the value is for).

Are you stuck with CSV or is JSON or XML an option? Either of those will let us use a transform and eliminate the need for the splits and trims.

Depending on the flexibility you have with the source, you could get this down to a single 3-6 line Rule and some configs.

If we assume there is no control over the source, there are still some things we can do, but I’ll need to see the CSV returned to tell you how to do it. It would look something like:

import java.util.List

val List<String> stocks = newArrayList("TW", "TCG", "SLA") // Each stock symbol

rule "Get share prices"
when
    Time cron "0 0/15 * ? * * *"
then
    stocks.forEach[ stock, i |
        createTimer(now.plusMinutes(i), [ |
            val csv = sendHttpGetRequest("https://www.alphavantage.co/query?function=TIME_SERIES_INTRADAY&symbol=LON:"+stock+"&interval=15min&outputsize=compact&datatype=csv&apikey=MYAPIKEY").split("\n")
            val firstLine = csv.get(2).split(",")
            val price = firstLine.get(2).trim
            logInfo("org.openhab", "Share Price Update: " + stock + "; " + price)
            sendCommand(stock+"_Current_Price", price)
        ])
    ]
end

Theory of operation:

The code above takes advantage of the fact that the only real difference between the three Rules is the stock symbol.

Put each stock symbol into a List.

Every 15 minutes loop through the list of stock symbols. For each symbol create a timer. Schedule the timer based on the index so the requests get spaced out by a minute. The first index is 0 so this timer will run immediately.

In the timer, call the URL, replacing the stock symbol with the one from the list and parse out the price. Log it, and use the stock symbol to update the Current_Price Item.

1 Like
5

@rlkoshak you sir, are a legend. That’s worked perfectly and means I can add more stares without adding more rules.

Thank you very much for your help with this. I’d searched the forum looking for examples and couldn’t find anything that worked. Hopefully others will find this useful.

6

Just keep the number of stocks under 15. If you exceed 15, you will need to adjust the Rule trigger so it runs every X minutes where X is > number of stocks in the list.

If you don’t you will end up with timers piling up over time and trying to run on top of each other and eventually you will run out of threads, and exceed your daily call limit.

7

Rather than creating a new topic I thought I reopen this one as it as useful information.

I’ve tried to increase the number of shares that I monitor, still less than 15. I now get the error below when I add a new share to the arraylist. I’m not sure what this means and therefore how I go about resolving the issue.

2020-04-22 16:01:12.894 [ERROR] [ntime.internal.engine.ExecuteRuleJob] - Error during the execution of rule 'Get The Latest Share Prices': 1

Many thanks in advance for your assistance.

8

None of the rules above are named that.

9

Well spotted, top marks for keeping me honest. Here’s the Rule in question, not much has change from those above apart from the name.

import java.util.List

val String filename = "Shares.rules"
val List<String> vShares = newArrayList("TW",
                                        "SLA",
                                        "RMG",
                                        "BARC",
                                        "VOD",
                                        "LLOY") //, "IAG", "ITV", "CNA", "GLEN") // Each share symbol

rule "Get The Latest Share Prices"
when
    Time cron "0 1/15 * ? * * *"
then
if(Weekend.state == ON) return;
if(now.getHourOfDay < 8 || now.getHourOfDay > 17) return;
    logInfo(filename,"Shares: Obtaining the latest prices.")
    vShares.forEach[ vShare, i |
        var csv = sendHttpGetRequest("https://www.alphavantage.co/query?function=TIME_SERIES_INTRADAY&symbol=LON:"+vShare+"&interval=15min&outputsize=compact&datatype=c
sv&apikey=APIKEY").split("\n")
        val firstLine = csv.get(2).split(",")
        val vOpeningPrice = firstLine.get(1).trim
        val vCurrentPrice = firstLine.get(2).trim
        logInfo(filename, "Shares: Updating {}.",vShare)
        sendCommand(vShare+"_Current_Price", vCurrentPrice)
        if(now.getHourOfDay > 7 && now.getHourOfDay < 9 && SOD_Update.state != ON) {
            logInfo(filename, "Shares: Setting the opening price for {} to be {}p", vShare, vOpeningPrice)
            sendCommand(vShare+"_SOD_Price", vOpeningPrice)
                SOD_Update.sendCommand(ON)
        }
        Thread::sleep(500)
    ]
logInfo(filename, "Shares: Updated completed.")
end
10

Rules DSL is sometimes unhelpful with messaging. I think it’s likely to do with an array that doesn’t have the requested element.
Most likely you’ve got a response from the http that doesn’t look like you expect and does not yield all the elements from the various split()s.
I’d add a diagnostic logInfo() to see each response.

Longer term you can make the rule more robust against that, so that the whole thing doesn’t fail if one share is duff.

11

So let me explain a little more. The rule above works fine. Has done for ages now, however if I add any of the additional shares the error appears. Which would suggest to me that it’s not the response back from the http request. I’ve also mixed and matched the shares, so long as there are only 6 in the list it just works. Once I add a seventh share the error appears.

12

My recommendation stands. Perhaps the target service allows you six hits a minute. But let’s not guess, find out.

13

Looks like your assumption was indeed right. I created a second list and added a second rule that runs a minute later and I’m no longer getting the error.

Thanks for the pointer.

14

In other words, you did not take the code @rlkoshak suggested, instead you do all the calls at once. In this case the advice to stay below 15 stocks doesn’t fit, as you have seen!

15

Not sure what you mean? I did follow the code that was suggested. The issue seems that I can only process 6 requests per minute. As you can see by my code I’m only monitoring 10 shares.

16

The suggested code used times, for each share a different timer would have been started. That way you would have had a maximum of one call per minute. Since the rule started every 15 minutes, only 15 calls could have been made.
Your code does perform all calls rigth after each other, hence the maximum calls per minutes strikes.