Clean Code – Is It Bad Style to Redundantly Check a Condition?

clean codecoding-style

I often get to positions in my code where I find myself checking a specific condition over and over again.

I want to give you a small example: suppose there is a text file which contains lines starting with "a", lines starting with "b" and other lines and I actually only want to work with the first two sort of lines. My code would look something like this (using python, but read it as pseudocode):

# ...
clear_lines() # removes every other line than those starting with "a" or "b"
for line in lines:
    if (line.startsWith("a")):
        # do stuff
    elif (line.startsWith("b")):
        # magic
    else:
        # this else is redundant, I already made sure there is no else-case
        # by using clear_lines()
# ...

You can imagine I won't only check this condition here, but maybe also in other functions and so on.

Do you think of it as noise or does it add some value to my code?

Best Answer

This is an excedingly common practice and the way of dealing with it is via higher-order filters.

Essentially, you pass a function to the filter method, along with the list/sequence that you want to filter against and the resulting list/sequence contains only those elements that you want.

I'm unfamiliar with python syntax (though, it does contain such a function as seen in the link above), but in c#/f# it looks like this:

c#:

var linesWithAB = lines.Where(l => l.StartsWith("a") || l.StartsWith("b"));
foreach (var line in linesWithAB)
{
    /* line is guaranteed to ONLY start with a or b */
}

f# (assumes ienumerable, otherwise List.filter would be used):

let linesWithAB = lines
    |> Seq.filter (fun l -> l.StartsWith("a") || l.StartsWith("b"))

for line in linesWithAB do
    /* line is guaranteed to ONLY start with a or b */

So, to be clear: if you use tried and tested code/patterns, it is bad style. That, and mutating the list in-memory the way you appear to via clear_lines() loses you thread safety and any hopes of parallelism that you could have had.

Related Topic