Never nest coding

🚧 Developers
1

This is neither Hubitat nor Groovy specific, but a friend turned me onto the so-called "never nest" aka "no indent" programming method. Really, more of a philosophy than a methodology. In short, never nest is to avoid if-else statements. After doing it for awhile, it just seems to intuitively better than the norm, tho it does take a bit of practice to get the full benefits. First, it's just easier to read, plain and simple. Second, it forces breaking out tricky code into their own function. Third, it pushes data validation to be "just in time". And fourth, it actually runs faster. Video: https://youtu.be/CFRhGnuXG-4 (not affiliated).

As an example, here's code of mine that takes a string for 'on', 'off', or 'toggle', and sets an atomicState variable for the device. It checks if the inputs are valid, if it's a change from the current setting, then if the atomicState exists, sets 'on' and 'off', then 'toggle'. Granted, it's not necessarily good code, even using traditional nesting, but that's actually the point. I have found using "never nest" forces my at best mediocre coding skills to be... better, maybe even good. It doesn't let me do spaghetti code.

def updateStateSingle(singleDevice,action){
    if(singleDevice && action){
        time = new Date().time
        if(atomicState?."deviceState${singleDevice.id}" && atomicState."deviceState${singleDevice.id}".'state'){
            if(!atomicState."deviceState${singleDevice.id}".'state' == action){
                if(action != 'toggle'){
                    atomicState."deviceState${singleDevice.id}" = ['state':action,'time':time]
                } else if(action == 'toggle'){
                    if(atomicState."deviceState${singleDevice.id}".'state' == 'on') {
                        atomicState."deviceState${singleDevice.id}" = ['state':'off','time':time]
                    } else if(atomicState."deviceState${singleDevice.id}".'state' == 'off') {
                        atomicState."deviceState${singleDevice.id}" = ['state':'on','time':time]
                    }
                }
            }      
        } else {
            if(action!= 'toggle'){
                atomicState."deviceState${singleDevice.id}" = ['state':action,'time':time]
            } else {
                atomicState."deviceState${singleDevice.id}" = ['state':'on','time':time]
            }
        }
    } else {
	log.debug 'ERROR'
    }
    return
}

Now, same code using "never indent":

if(!updateStateSingle(singleDevice,action) log.debug 'ERROR'
def updateStateSingle(singleDevice,action){
    if(!singleDevice) return
    if(!action) return

    time = new Date().time
    if(!atomicState?."deviceState${singleDevice.id}") atomicState."deviceState${singleDevice.id}" = [:]
    if(action == 'toggle'){
        if(!atomicState?."deviceState${singleDevice.id}"?.'state') action == 'on'
        if(atomicState."deviceState${singleDevice.id}".'state' == 'on') action == 'off'
        if(atomicState."deviceState${singleDevice.id}".'state' == 'off') action == 'on'
    }
    if(atomicState."deviceState${singleDevice.id}".'state' == action) return	// No change required
    atomicState."deviceState${singleDevice.id}" = ['state':action,'time':time]
    return true
}

It does virtually the same thing, but a lot fewer lines (only difference being the original did NOT check if the map key exists). I'm actually embarrassed for the first code. It's terrible. But as bad as my coding skills might be to produce that, the same level of skill did the second. Aside from being cleaner, easier to debug, etc., it self-prompts me to create smaller functions. In this example, the nest (for setting up 'toggle') suggests maybe I should break it out.

if(!updateStateSingle(singleDevice,action) log.debug 'ERROR'
def updateStateSingle(singleDevice,action){
    if(!singleDevice) return
    if(!action) return

    time = new Date().time
    if(!atomicState?."deviceState${singleDevice.id}") atomicState."deviceState${singleDevice.id}" = [:]
    action = getUpdateStateSingleToggle(singleDevice,action)
    if(atomicState."deviceState${singleDevice.id}".'state' == action) return	// No change required
    atomicState."deviceState${singleDevice.id}" = ['state':action,'time':time]
}
def getUpdateStateSingleToggle(singleDevice,action){
    if(action != 'toggle') return action   // If original value is already 'on' or 'off', preserves it
    if(!atomicState?."deviceState${singleDevice.id}"?.'state') return 'on'
    if(atomicState."deviceState${singleDevice.id}".'state' == 'on') return 'off'
    if(atomicState."deviceState${singleDevice.id}".'state' == 'off') return 'on'
}

edit: Disclaimer on the example.... The error handling isn't great in either version. It does not enforce 'on', 'off' or 'toggle'. On the first, could do 'if(action != on || off || toggle)" at the top. With nesting, could just check "on || off" after doing the 'toggle' state, because without nests, it doesn't have to be in the right branch(es) of a if-then-else.

3

Oops. Using a computer that doesn't have YT access, so typed it out. Try again.

edit: Fixed for reals.

1 Like
4

The video above shows creating smaller functions, but it doesn't really show usefulness of so-called "inversion". Here's a video with that (it's python, which uses indentation rather than brackets): https://youtu.be/NCnPg_S_Eu8.

However, never indenting really shines when combing "extraction" and "inversion" together, by allowing functions to be self-validating. From the first video:

def processDownload(isIterator = DownloadState> iterator){
     if(download.getState() == downloadState.State.InProgress){
          if (processInProgress(Download)){
               return true;
          }
    }
     if(download.getState() == downloadState.State.Pending){
          if (processPending(Download)){
               return true;
          }
     }
    [etc]
}
def processInProgress(Download){
     // do stuff
}
def processPending(Download){
     // do stuff
}

Instead, I would do something like this:

def processDownload(isIterator = DownloadState> iterator){
     if(processInProgress(Download)) return true
     if(processPending(Download)) return true
     [etc.]
}
def processInProgress(Download){
     if(download.getState() != downloadState.State.InProgress) return
     // do stuff
    return true
}
def processPending(Download){
     if(download.getState() != downloadState.State.Pending) return
     // do stuff
    return true
}

By shifting the conditions for the function inside the function, then simply checking the function boolean is good enough, and allows calling all the subfunctions. Since each is mutually exclusive, they will return false if the condition isn't met and go to the next.

5

There are many ways to skin a cat. I've never been a fan of multiple return statements. One way in, one way out is my preference.

We don't have a debugger so in the context of HE it doesn't matter, but if there's only one return statement, you only need to place a single breakpoint. But it does make placing trace logs like "foo() entered" and "foo() exited" a pain as well.

2 Likes
6

"foo() entered" and "foo() exited" a pain as well.

That is true. With debugging tools, unit testing subfunctions mostly would offset that. But without debugging, I personally find that to be outweighed by being able to shift the code around. Within nests, that's a pain at best. See below on a mock-up of my testing tricks made easier with "never indent" (and certainly could still be done with single entry-exit functions, just messier).

def main(){
    stateValue = checkState(settings['state'])
    if(!stateValue) {
           log.error "No or bad value for settings['state'] $settings['state']"
           return
     }
    if(!setMap(stateValue)) {
           log.error "Map error, check if it exists with  $settings['state'] -- atomicState $atomicState?.['devices']"
           return
     }
    //map = getStateMap()     // commented for testing - UNCOMMENT THIS
    map = ['state':'on']       // Set fake testing value - REMOVE THIS
    if(!setDeviceToMap(map)) {
           log.error "Can't update device $settings['device']"  // Oops, need to add a check to test it's a switch
           return
     }
}
def checkState(stateValue){
    // Validates/normalizes settings['state']
    // If null or "bad", returns false
    // Otherwise, return normalized value
    return stateValue
}
def setStateMap(stateValue){
    if(!stateValue) return
    // Puts settings['state'] (etc) into atomicState map
}
def getStateMap(){
    if(!atomicState?.'device'?.'state') return
    return atomicState.'device'.'state'
}
def setDeviceToMap(map){
    // Set device to match values in atomicState
}