Validation function using both for-loops and indexes at the same time [closed]

Question

This function iterates through a open dialog box that has 4 values in it all optional except for aircraft1, aircraft2, warning_or_alert.

1. aircraft1, aircraft2
2. t_sep # time value in format 00:12:21
3. t_min_nm # time value in format 00:21:21
4. c_tracks # can be either 'on' or 'off'
5. warning_or_alert # can be either 'warning' or 'alert'

First I check if I have correct airplanes aircraft_str = aircraft1 + ' - ' + aircraft2. If I have correct aircraft I check if I have warning_or_alert. If that is correct I can validate the last three variables t_sep, t_min_nm, c_tracks. Don't worry about errorhandling.stca_list_exception; it's a function in a different module that handles me error as well is utils.time_in_range that function compares two times.

My concern here is my looping technic, by having for loops but I'm using indexes instead of the for loop functionality utils.time_in_range(t_sep, squishtest.object.children(si)[i+2].text) for instance.

def stca_list(aircraft1=None, aircraft2=None, t_sep=None, t_min_nm=None, c_tracks=None, warning_or_alert):
    aircraft_str = None
    if aircraft1:
        aircraft_str = aircraft1 + ' - ' + aircraft2
    stca_list_dialog = squishtest.waitForObject("{type='isds::StcaListDialog' unnamed='1' visible='1' windowTitle='STCA List'}")
    for si in squishtest.object.children(stca_list_dialog):
        if squishtest.className(si) == 'QTableWidget':
            for i, ssi in enumerate(squishtest.object.children(si)):
                if squishtest.className(ssi) == 'QModelIndex':
                    for stca_list_value in squishtest.object.children(si)[i+1:]:
                        if aircraft_str == stca_list_value.text:
                            if warning_or_alert == 'warning':
                                if stca_list_value.foregroundColor == '#FFFFFF00': # yellow
                                    if t_sep:
                                        utils.time_in_range(t_sep, squishtest.object.children(si)[i+2].text)

                                    if t_min_nm:
                                        utils.time_in_range(t_min_nm, squishtest.object.children(si)[i+3].text)

                                    if c_tracks and c_tracks != squishtest.object.children(si)[i+4].text:
                                        raise AssertionError("Error CTracks: Expected[%s] got[%s]"c_tracks, squishtest.object.children(si)[i+4].text)

                                    squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))
                                    return
                                else:
                                    errorhandling.stca_list_exception('Error Warning: expected[warning] got[alert]')
                            elif warning_or_alert == 'alert':
                                if stca_list_value.foregroundColor == '#FFFF0000': # red
                                    if t_sep:
                                        utils.time_in_range(t_sep, squishtest.object.children(si)[i+2].text)

                                    if t_min_nm:
                                        utils.time_in_range(t_min_nm, squishtest.object.children(si)[i+3].text)

                                    if c_tracks and c_tracks != squishtest.object.children(si)[i+4].text:
                                        raise AssertionError("Error CTracks: Expected[%s] got[%s]"c_tracks, squishtest.object.children(si)[i+4].text)

                                    squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))
                                    return
                                else:
                                    errorhandling.stca_list_exception('Error alert: expect[alert] got[waning]')
                            else:
                                errorhandling.stca_list_exception('Warning or Alert Error: Choose either warning or ellert, [%s] does not exis.' % warning_or_alert)
                        else:
                            errorhandling.stca_list_exception('Are value but wrong expected[%s] but was[%s]' % (aircraft_str, stca_list_value.text))

    if aircraft_str:
        errorhandling.stca_list_exception('Error: No warnings or alerts, but got aircraft[%s] and aircraft[%s] and warning or alert[%s]' % (aircraft1, aircraft2, warning_or_alert))

    squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))

Answer 1

The reason that this is so complex is that instead of doing the sequence "find widget in question" - "process it", the code is doing something like "iterate over all widgets" - "if one is the widget in question, process it and exit", so it ends up with a lot of nesting instead of a flat sequence of steps.

So, I'd rewrite it to first, find the object, one with class QTableWidget, which has a child with class QModelIndex, which has exactly one entry with some specific text. Then, use that entry and do some assertions.

---

Some more general remarks:

• The variable names aren't the best, i.e. not descriptive of the kinds of values they contain.
• The messages have some typos; I've changed them where I could.
• Maybe import more; the constant repetitions of errorhandling and squishtest are quite verbose.
• aircraft_str could be initialised on one line like aircraft_str = aircraft1 and '{} - {}'.format(aircraft1, aircraft2) or even with aircraft_str = '{} - {}'.format(aircraft1, aircraft2) if aircraft1 else None. If you're consistently using % for string formatting of course use that instead.
• The test for aircraft_str at the end should be if aircraft1: instead, because that is directly related to one of the input variables and aircraft_str is more formatting than logic IMO.
• Exit loops early, don't nest so much. E.g. if you have a single if, invert the test and continue in a loop.
• The use of the index i in that way is error prone and, depending on what squishtest.object.children(si) does, costly. IMO, since you access by index anyway, don't use enumerate then, but destructure the first five items at the start of the corresponding loop and reuse the variables (and give them nice names so we know what the widgets are).
• Input verification should be done early. So if warning_or_alert can have one of two values, the check for that should come once, before the rest of the function (the value could also be cached for faster execution and less code duplication). That way not only is the test not execute too often (that is, more than once), but it is very clear that the variable can only have a valid value for the rest of the function, so reasoning about the state is much easier. It's also much more self-documenting when reading the source. An actual raise stca_list_exception would also make clear that this raises an exception.
• The colours are documented, but I'd still move them into separate variables to aid readability.
• Both branches of the if switch on warning_or_alert are almost identical. That should factored out. The AssertionError is also weirdly called, it looks like the string formatting is actually missing there. E.g. why not have a expected_color variable so that it can be checked without branching?

With all that in mind, it could look like this, note the nested loop is still there, but it already looks much cleaner:

def stca_list(aircraft1=None, aircraft2=None, t_sep=None, t_min_nm=None, c_tracks=None, warning_or_alert):
    aircraft_str = aircraft1 and '{} - {}'.format(aircraft1, aircraft2)

    if warning_or_alert not in ('warning', 'alert'):
        errorhandling.stca_list_exception('Warning or Alert Error: Choose either warning or alert, [%s] does not exist.' % warning_or_alert)

    is_warning = warning_or_alert == 'warning'
    expected_color = YELLOW if is_warning else RED

    stca_list_dialog = squishtest.waitForObject("{type='isds::StcaListDialog' unnamed='1' visible='1' windowTitle='STCA List'}")

    for si in squishtest.object.children(stca_list_dialog):
        if squishtest.className(si) != 'QTableWidget':
            continue

        children = squishtest.object.children(si)

        for i, ssi in enumerate(children):
            if squishtest.className(ssi) != 'QModelIndex':
                continue

            for stca_list_value in children[i+1:]:
                if aircraft_str != stca_list_value.text:
                    errorhandling.stca_list_exception('Are value but wrong expected[%s] but was[%s]' % (aircraft_str, stca_list_value.text))

                if stca_list_value.foregroundColor != expected_color:
                    errorhandling.stca_list_exception('Error Warning: expected[%s] got[%s]' % ('alert' if is_warning else 'warning', warning_or_alert)

                if t_sep:
                    utils.time_in_range(t_sep, children[i+2].text)

                if t_min_nm:
                    utils.time_in_range(t_min_nm, children[i+3].text)

                if c_tracks and c_tracks != children[i+4].text:
                    raise AssertionError("Error CTracks: Expected[%s] got[%s]" % (c_tracks, children[i+4].text))

                squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))
                return

    if aircraft1:
        errorhandling.stca_list_exception('Error: No warnings or alerts, but got aircraft[%s] and aircraft[%s] and warning or alert[%s]' % (aircraft1, aircraft2, warning_or_alert))

    squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))

However having the assertions in the loop still looks bad and there's the return and the duplicated "waiting for click on close button". So now I'd move the finding of the objects in question out of there to simplify the logic. The function will return the four widgets if it matches the condition, otherwise it will (by default) return None:

def find_stca_list_value():
    stca_list_dialog = squishtest.waitForObject("{type='isds::StcaListDialog' unnamed='1' visible='1' windowTitle='STCA List'}")

    for si in squishtest.object.children(stca_list_dialog):
        if squishtest.className(si) != 'QTableWidget':
            continue

        children = squishtest.object.children(si)

        for i, ssi in enumerate(children):
            if squishtest.className(ssi) != 'QModelIndex':
                continue

            return children[i+1:i+4]

Waiting for the close button can also be a separate function, or it could be nested in stca_list as well:

def wait_for_close_button():
    squishtest.clickButton(squishtest.waitForObject("{name='CloseButton' text='Close' type='QPushButton' visible='1'}"))

Now the actual function is reasonably short and the logic is easier to see. children1 to children3 still need better names!

def stca_list(aircraft1=None, aircraft2=None, t_sep=None, t_min_nm=None, c_tracks=None, warning_or_alert):
    if warning_or_alert not in ('warning', 'alert'):
        errorhandling.stca_list_exception('Warning or Alert Error: Choose either warning or alert, [%s] does not exist.' % warning_or_alert)

    result = find_stca_list_value()

    if not result:
        if aircraft1:
            errorhandling.stca_list_exception('Error: No warnings or alerts, but got aircraft[%s] and aircraft[%s] and warning or alert[%s]' % (aircraft1, aircraft2, warning_or_alert))

        wait_for_close_button()
        return

    stca_list_value, children2, children3, children4 = result

    aircraft_str = aircraft1 and '%s - %s' % (aircraft1, aircraft2)
    if aircraft_str != stca_list_value.text:
        errorhandling.stca_list_exception('Are value but wrong expected[%s] but was[%s]' % (aircraft_str, stca_list_value.text))

    is_warning = warning_or_alert == 'warning'
    if stca_list_value.foregroundColor != (YELLOW if is_warning else RED):
        errorhandling.stca_list_exception('Error Warning: expected[%s] got[%s]' % ('alert' if is_warning else 'warning', warning_or_alert))

    if t_sep:
        utils.time_in_range(t_sep, children2.text)

    if t_min_nm:
        utils.time_in_range(t_min_nm, children3.text)

    if c_tracks and c_tracks != children4.text:
        raise AssertionError("Error CTracks: Expected[%s] got[%s]" % (c_tracks, children4.text))

    wait_for_close_button()