1

This question is specifically regarding coding convention. I know that using if or elif in this case will produce the same results. Just wondering which is the "proper" way to construct this function:

With consecutive if:

def can_take(self, selectedCourse):
    if selectedCourse.hasPassed():
        return False
    if selectedCourse.getPrereqs() != 'none':
        for prereq in selectedCourse.getPrereqs():
            if not self.courses[prereq].hasPassed():
                return False

    return True

With elif:

def can_take(self, selectedCourse):
    if selectedCourse.hasPassed():
        return False
    elif selectedCourse.getPrereqs() != 'none':
        for prereq in selectedCourse.getPrereqs():
            if not self.courses[prereq].hasPassed():
                return False

    return True
NewBoard
  • 180
  • 1
  • 9
  • do u want to fire both of the code under both the if statements? then you can do use only if, if u don't want to fire both of the if statements, then use elif – Ghost Ops Sep 25 '21 at 13:51
  • @GhostOps You can't do that; if the first is True, the function returns immediately. As a personal preference, I would use two `if` statements. – chepner Sep 25 '21 at 13:55
  • 2
    As an aside, this code can be made simpler if `getPrereqs()` returns an empty list rather than `none` if there are no prerequisites. – chepner Sep 25 '21 at 13:59
  • @chepner I would love for that to be the case. Unfortunately I'm only able to modify some of the modules in this project. So I have to work with 'none' rather than [] lol – NewBoard Sep 25 '21 at 14:04

4 Answers4

5

If I had to choose between the two, I would probably use two if statements, but that's just a matter of personal preference.

If I had a third choice, I wouldn't have any return statements with Boolean literals. I would write a single return statement that uses and and or.

return (not selected.hasPassed()
        and (selected.getPrereqs() == 'none'
             or all(x.hasPassed() 
                    for x in selected.getPrereqs()))

This is close to how you would describe this in English: you can take the class if you have not passed it, and if the class either has no prerequisites or if you have passed all the prerequisites.

As John Kugelman points out, if getPrereqs returned an empty list instead of 'none', you could further reduce this to

return (not selected.hasPassed()
        or all(x.hasPassed() 
               for x in selected.getPrereqs())
chepner
  • 497,756
  • 71
  • 530
  • 681
  • Thank you so much for this. The single return statement is so elegant. I did not know of the all() function, will definitely be making use of that in the future. – NewBoard Sep 25 '21 at 15:03
3

I love the early return pattern:

Get invalid cases out of the way first, either simply exiting or raising exceptions as appropriate, put a blank line in there, then add the "real" body of the method. I find it easier to read.

Returning early keeps the nesting level down, which is great way to reduce cognitive load. I would take it one step further and flip the second if statement around so it too returns early:

def can_take(self, selectedCourse):
    if selectedCourse.hasPassed():
        return False

    if selectedCourse.getPrereqs() == 'none':
        return True

    for prereq in selectedCourse.getPrereqs():
        if not self.courses[prereq].hasPassed():
            return False

    return True

That said, some other improvements I would make:

  • Avoid stringly typed variables. Switch that 'none' to None.

  • But then, when a method returns a list don't return None when there are no results. Return an empty list. Then the caller can blindly iterate over the list without checking if it's None or empty.

def can_take(self, selectedCourse):
    if selectedCourse.hasPassed():
        return False

    for prereq in selectedCourse.getPrereqs():
        if not self.courses[prereq].hasPassed():
            return False

    return True

If you're comfortable with generator expressions you could even convert the loop into an all(...) call, removing the need for the final return True.

def can_take(self, selectedCourse):
    if selectedCourse.hasPassed():
        return False

    return all(self.courses[prereq].hasPassed()
        for prereq in selectedCourse.getPrereqs())

I like this because it's a more direct encoding of the question: "Has the student passed all of the prereqs?"

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
  • Thank you for this. I actually really like this structure. I originally thought about doing it this way, but then I still need to return True at the end of the function if the for loop never returns False. So in the end, I'm left with an extra line of code doing it this way. – NewBoard Sep 25 '21 at 14:08
  • Also, as I stated above. I'm only able to modify some of the modules in this project. I would love to not be working with 'none', but unfortunately I have to for my assignment. – NewBoard Sep 25 '21 at 14:11
1

I think I prefer the first version. Why? When you have an if...elif...elif... thing with returns in each branch, there are two "competing" control structures: the if statement and the returns. Obviously, the returns will "win", so we might as well remove the elif stuff. Either that, or have just one return statement, which returns a value computed by a preceding if...elif...elif...else structure.

Ture Pålsson
  • 6,088
  • 2
  • 12
  • 15
  • Thank you for this answer. This was my thinking as well. Just wasn't sure if it was convention to use 'elif' for readability or something else. – NewBoard Sep 25 '21 at 14:06
  • Another way of looking at it is that the two are only equivalent because the code after the `if/elif` consists only of another `return`. In fact, both cases have already dropped the implicit `else` that would have returned `True`, so it makes sense to "strip" the `else` from `elif` as well. – chepner Sep 25 '21 at 14:17
0

We use elif but please understand it depends on your problem statement.

Note: Please do not create a big ladder out of it as then it becomes difficult to maintain and debug the code.

VMSMani
  • 416
  • 4
  • 7