[Resolved] Coding advice and guidance - to many if statements?

Setup, Configuration and Use Scripts & Rules
1

Hi Everyone,

I’m once again after assistance with my poor coding skills. Below is a rule I have in place, it’s in a logical order but I fear not the cleverest way of achieving my goal.

The aim of the rule is two fold. Firstly switch on my Kitchen light when the motion sensor is activated, the ON state. The second and more interesting part is to announce the latest weather information and train status via my Sonos speaker. The announcement only needs to be done the first time I enter the Kitchen and as I don’t work weekends there is no need for any train status then.

As you can see I’ve used a number of IF, ELSE statements. This works fine but isn’t likely to be the best method.

Any help and advice would be really appreciated.

rule "Kitchen Motion Sensor Activation"
when
	Item Kitchen_SP3102_Motion changed from OFF to ON
then
	postUpdate(Kitchen_Motion_Sensor_Last_Activation, new DateTimeType())
	if(Time_Of_Day.state == "MORNING" || Time_Of_Day.state == "DAY") {
		if(Outside_ST815_Lux_Level.state < 100) {
			Kitchen_Strip_Light_Dimmer.sendCommand(35)
		}
	} else if(Time_Of_Day.state == "EVENING" && Hallway_SP3102_Motion.state == ON) {
		Kitchen_Strip_Light_Dimmer.sendCommand(65)
	} else if(Time_Of_Day.state == "NIGHT" && Hallway_SP3102_Motion.state == ON) {
		Kitchen_Strip_Light_Dimmer.sendCommand(35)
	} else if(Time_Of_Day.state == "BED" && Hallway_SP3102_Motion.state == ON) {
        Kitchen_Strip_Light_Dimmer.sendCommand(25)	
	}
	
	var String Morning_Brief = null
	if(Is_It_The_Weekend.state == "NO" && Time_Of_Day.state == "MORNING" || Time_Of_Day.state == "DAY" && Morning_Announced.state != "Yes") {
		
		if(Weather_Current_Min_Temp.state > 0) {
			Morning_Brief = "Good Morning, The Morning Train is Currently;" + Morning_Train_Status.state + ". Today the expected high is;" + Weather_Current_Max_Temp.state + "degrees C." + ". The forecast for today is;" + Weather_Current_Condition.state
		} else if(Weather_Current_Min_Temp.state < 0) {
			Morning_Brief = "Good Morning, The Morning Train is Currently;" + Morning_Train_Status.state + ". Today is likely to be cold the expected high is;" + Weather_Current_Max_Temp.state + "degrees C." + ". With an expected low of;" + Weather_Current_Min_Temp.state + "degrees C." + ". The forecast for today is;" + Weather_Current_Condition.state
		}
	} else if(Is_It_The_Weekend.state == "YES" && Time_Of_Day.state == "MORNING" || Time_Of_Day.state == "DAY" && Morning_Announced.state != "Yes") {
		val String Morning_Brief = "Good Morning, Today's high is expected to be;" + Weather_Current_Max_Temp.state + "degrees C." + ". The forecast is;" + Weather_Current_Condition.state
	}
	setMasterVolume(new PercentType(20))
	say(Morning_Brief)
	postUpdate(Morning_Announced, "Yes")
end
2

The first thing I can suggest is Design Pattern: How to Structure a Rule. This can help simplify things a bit.

You should be using Switch or Contact Items for binary Items like Is_It_The_Weekend and Morning_Announced.

That would take the Rule to something like:


    // 1. Always run the rule

    // 2. Calculate what needs to be done
    // calculate the dimming state
    val dimmingState == -1

    switch Time_Of_Day.state {
        case "Morning",
        case "DAY": if(Outside_Str815_Lux_Level.state < 100) dimmingState = 35
        case "EVENING": dimmingState = 65
        case "NIGHT": dimmingState = 35
        case "BED": dimmingState = 25
    }

    // calculate the morning brief
    var giveBrief = Time_Of_Day.state == "MORNING" || 
                    Time_Of_Day.state == "DAY" && Morning_Announced.state != ON
    var briefing = "Good morning, [0] " + 
                   " [1] the expected high is; " + Weather_Current_Max_Temp.state + 
                   " degrees C. [2]. The forecast for today is; " + Weather_Current_Condition.state
    if(giveBrief) {
        if(Is_It_The_Weekend.state == OFF){
            briefing = briefing.replace("[0]", "The Morning Train is Currently; " + Morning_Train_Status.state

            if(Weather_Current_Min_Temp.state > 0) briefing.replace("[1]", "Today").replace("[2]", "")
            else briefing.replace("[1]", "Today is likely to be cold the").replace("[2]", " With an expected low of; " + Weather_Current_Min_Temp.state + " degrees C."
        }
        else{
            briefing = briefing.replace("[0]", "").replcace("[1]", "Today").replace("[2]", "")
        }
    }

    // 3. Do it
    if(dimmingState != -1) Kitchen_Strip_Light_Dimmer.sendCommand(dimmingState)
    if(giveBrief) {
	setMasterVolume(new PercentType(20))
	say(Morning_Brief)
	postUpdate(Morning_Announced, "Yes")
    }
end

I’m not certain this simplifies it much but it does reduce some of the indentations. It is more lines but that is because we broke up the construction of the briefing into multiple lines. Since the messages are mostly the same, I use a find and replace approach to build up the briefing.

You could split the rule up into multiple rules which might help:

rule "Kitchen Motion Lights"
when 
    Item Kitchen_SP3102_Motion changed from OFF to ON
then
    val dimmingState == -1

    switch Time_Of_Day.state {
        case "Morning",
        case "DAY": if(Outside_Str815_Lux_Level.state < 100) dimmingState = 35
        case "EVENING": dimmingState = 65
        case "NIGHT": dimmingState = 35
        case "BED": dimmingState = 25
    }

    if(dimmingState != -1) Kitchen_Strip_Light_Dimmer.sendCommand(dimmingState)
end

rule "Kitchen Motion Briefing"
when
    Item Kitchen_SP3102_Motion changed from OFF to ON
then
    if(Time_Of_Day.state != "MORNING" || Time_Of_Day.state != "DAY") return;
    if(Time_Of_Day.state == "DAY" && Morning_Announced.state == ON) return;

    var briefing = "Good morning, [0] " + 
                   " [1] the expected high is; " + Weather_Current_Max_Temp.state + 
                   " degrees C. [2]. The forecast for today is; " + Weather_Current_Condition.state
    if(Is_It_The_Weekend.state == OFF){
        briefing = briefing.replace("[0]", "The Morning Train is Currently; " + Morning_Train_Status.state

        if(Weather_Current_Min_Temp.state > 0) briefing.replace("[1]", "Today").replace("[2]", "")
        else briefing.replace("[1]", "Today is likely to be cold the").replace("[2]", " With an expected low of; " + Weather_Current_Min_Temp.state + " degrees C."
    }
    else{
        briefing = briefing.replace("[0]", "").replcace("[1]", "Today").replace("[2]", "")
    }

    setMasterVolume(new PercentType(20))
    say(Morning_Brief)
    postUpdate(Morning_Announced, "Yes")    
end

By splitting them up you can take care of a bunch of the cases where you don’t need to say a briefing up front so you don’t have to deal withthem later.

3

Thanks once again @rlkoshak

I’ve gone down the route of separating the rule into two as suggested. You given me more of an understanding now on how to remove all those if statements. I’m not overly fussed by the number of lines in the rule as long as it works and looks neat and tidy as per your refactoring above.

I did come across one issue though with this line not working. It kept creating an error in the openhab.log. I’m not in front of my server so can’t paste the exact error.

val dimmingState == -1

It seemed to work OK when I change to this instead.

var dimmingState == -1

I can now look at removing even more of my IF statements.

4

Yes, it need to be a var. I use val be default and sometimes forget to change it back to var when the variable gets reassigned like it does in this rule.

5

From the advice given above I’ve now reformatted one of my rules, below.

All seams to be working well apart from the very last two else if statements. They don’t run even when the light level is at 0 (zero). My thinking is that I need to move those two statements higher up the order, but would like confirmation that I’m on the right track or not.

As always I appreciate your input and advice.

rule "Ground Floor Lights"
when
	Item Outside_ST815_Lux_Level changed or
	Item Living_Room_SP3102_Motion changed from OFF to ON
then
	if(now.getHourOfDay < 9 || Time_Of_Day.state == "MORNING") return;
	if(Outside_ST815_Lux_Level.state > 120) return;
	if(Living_Room_SP3102_Motion.state == ON) {
		postUpdate(Living_Room_Motion_Sensor_Last_Activation, new DateTimeType())
	}
	if(Outside_ST815_Lux_Level.state > 110) {
		Living_Room_Floor_Lamp_Switch.sendCommand(OFF)
		Living_Room_Table_Lamp_Switch.sendCommand(OFF)
		Living_Room_Twig_Lights.sendCommand(OFF)
		Kitchen_Bloom_Lamp_Switch.sendCommand(OFF)
		logInfo("Living Room Lights", "Lights Out because the light level is abouve 110 Lux")
	} else if(Outside_ST815_Lux_Level.state > 90 && Living_Room_TV_Switch.state == ON) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(40)
		Living_Room_Table_Lamp_Dimmer.sendCommand(50)
		Kitchen_Bloom_Lamp_Dimmer.sendCommand(100)
		logInfo("Living Room Lights", "Lights on stage 1, light level is above 90 Lux")
	} else if(Outside_ST815_Lux_Level.state > 70 && Living_Room_TV_Switch.state == ON) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(50)
		Living_Room_Table_Lamp_Dimmer.sendCommand(75)
		Living_Room_Twig_Lights.sendCommand(ON)
		logInfo("Living Room Lights", "Lights on stage 2, light level is above 70 Lux")
	} else if(Outside_ST815_Lux_Level.state < 50 && Living_Room_TV_Switch.state == ON) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(60)
		Living_Room_Table_Lamp_Dimmer.sendCommand(100)
		logInfo("Living Room Lights", "Lights on stage 3, light level below 50 Lux. TV On")
	} else if(Outside_ST815_Lux_Level.state < 50 && Living_Room_TV_Switch.state == OFF) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(40)
		Living_Room_Table_Lamp_Dimmer.sendCommand(50)
		Kitchen_Bloom_Lamp_Dimmer.sendCommand(100)
		logInfo("Living Room Lights", "Lights on stage 4, light level below 50 Lux. TV Off")
	} else if(Outside_ST815_Lux_Level.state < 1 && Living_Room_TV_Switch.state == ON) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(60)
		Living_Room_Table_Lamp_Dimmer.sendCommand(100)
		Living_Room_Twig_Lights.sendCommand(ON)
		logInfo("Living Room Lights", "Lights on stage 5, light level is 0 and TV On")
	} else if(Outside_ST815_Lux_Level.state < 1 && Living_Room_TV_Switch.state == OFF) {
		Living_Room_Floor_Lamp_Dimmer.sendCommand(40)
		Living_Room_Table_Lamp_Dimmer.sendCommand(50)
		Kitchen_Bloom_Lamp_Dimmer.sendCommand(100)
		logInfo("Living Room Lights", "Lights on stage 6, light level is 0 and TV Off")
	}
end
6

Does your sensor measure the Lux level ever down below 1? Depending where the sensor is and where you are living it might not get that dark.

7

You do need to move them up. When using < then the smallest comparisons need to be first. When the level is 1 then the case where the level is < 50 matches so that else if runs instead.

8

Thanks for the confirmation, all working now.