This rule works but should it need improvement

Setup, Configuration and Use Scripts & Rules
1

Hi all
Got this rule. It works, but I think it needs improvement. Am I right?

rule "Heater Gameroom"
when
    Item GameRoom_temp changed
then
    val Integer currentHour = now.getHour
    if (GameRoom_temp.state < 11 | ℃ && Heater_Game_Room_Switch.state == OFF) {
        Heater_Game_Room_Switch.sendCommand(ON)
        // Temp komt onder de 11 graden
        logInfo("Temp onder 11", "Send On " )
    } else if (GameRoom_temp.state > 19 | ℃ && Heater_Game_Room_Switch.state == ON && (14..20).contains(currentHour) ) {
        Heater_Game_Room_Switch.sendCommand(OFF)
        logInfo("Temp +19", "Send Off " )
        // Schakelaar gaat uit tussen 14 en 20 wanneer temp 19 is
    } else if (GameRoom_temp.state > 14 | ℃ && Heater_Game_Room_Switch.state == ON && (20..14).contains(currentHour) ) {
        Heater_Game_Room_Switch.sendCommand(OFF)
        logInfo("Temp +14", "Send Off " )
       // Schakelaar gaat uit tussen 20 en 14 wanneer temp 14 is
}
end
2

Well, it’s not that bad :slight_smile: However:

rule "Heater Gameroom"
when
    Item GameRoom_temp changed
then
    val Boolean bComfort = (14..20).contains(now.getHour)      // true between 14:00:00 and 20:59:59
    var nNorm = Heater_Game_Room_Switch.state                  // get state once
    if(newState < 11 | ℃) {                                   // Temp komt onder de 11 graden
        nNorm = ON
        logInfo("heat", "Temp onder 11, send On")
    } else if(bComfort && newState > 19 | ℃) {                // Schakelaar gaat uit tussen 14 en 21 wanneer temp 19 is
        nNorm = OFF
        logInfo("heat", "Temp +19, send Off")
    } else if(!bComfort && newState > 14 | ℃) {               // Schakelaar gaat uit tussen 21 en 14 wanneer temp 14 is
        nNorm = OFF
        logInfo("heat", "Temp +14, send Off")
    }
    if(Heater_Game_Room_Switch.state != nNorm)                 // switch only if needed
        Heater_Game_Room_Switch.sendCommand(nNorm.toString)
end

Check the state of the switch two times instead of three times (checking the local var is much faster)
Check the time once instead of three times (again… boolean operation with a local var…)
Check newState rather than GameRoom_temp.state because again, newState is local val and on top of that (although very unlikely) GameRoom_temp.state can change while the rule is running, but newState not.
Use logInfo() the correct way (i.e. the first string is the logger name, it’s to control the logging itself during runtime)

2 Likes
3

Some improvements on the improvements. :smiley:

Really, the only thing I have is to make the log statements contain more information. You don’t really need to know what the rule did, you can get that information from the events.log. But what you want to know is why the rule did what it did, so I would add more information to the logs, including newState and bComfort.

For copy and pasters, be careful of that °C, it looks a little odd and might be some other unicode.

℃ != °C

And one minor quibble is I’d remove the Boolean from the declaration of bComfort.

Just in case it comes up in the future, this is unlikely to work the way you think it will. I think it actually is the same as (14..20).contains(currentHour). That’s what makes @Udo_Hartmann’s approach a good one. Either it’s between 14:00 and 20:00 or it’s not so there is no need to test if the hour is 00:00 to 13:00 or from 21:00-23:00 which is what I think was your intent.

3 Likes
4

Thank you @Udo_Hartmann and @rlkoshak for clarifying this a bit for me. Very useful.
As it concerns this rule writing I have the feeling there are so many ways to approach. Also many different things one can use. I find it sometimes hard to know which one is the best or which one I’m supposed to include in my rule.
Thankfully there is this forum and the help of people when you’re out of options.
Also this post helps me to get a bit more understanding Making OH Rules Easier for Everyone - #81 by rlkoshak