Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
5  
It's basically about whether or not you are coding defensively. Do you see this code being edited a lot? Is it likely that this is going to be a part of a system which needs to be extremely reliable? I don't see much harm in shoving an assert() in there to help with testing, but beyond that is probably excessive. That said, it'll vary depending on the situation. –  Lattyware Nov 5 '12 at 1:58
    
your 'else' case is essentially dead / unreachable code. Do check that there are no system wide requirements that prohibit this. –  NWS Nov 5 '12 at 9:27
2  
If I had a penny for every time I found an "impossible" bug... –  Jaydee Nov 5 '12 at 9:42
    
@NWS: are you saying that I should keep the else case? Sorry I don't understand you completely. –  mcwise Nov 5 '12 at 11:18
2  
not especially related to the question - but I would make that 'assertion' into an invariant - which would require a new "Line" class (perhaps with derived classes for A & B), rather than treating the lines as strings and telling them what they represent from the outside. I'd be happy to elaborate on this over at CodeReview –  MattDavey Nov 5 '12 at 11:19

7 Answers 7

up vote 14 down vote accepted

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.

share|improve this answer
3  
As a note, the python syntax for this would be a generator expression: (line for line in lines if line.startswith("a") or line.startswith("b")). –  Lattyware Nov 5 '12 at 10:44
    
This looks really elegant. Thanks! –  mcwise Nov 5 '12 at 11:24
1  
+1 for pointing out that the (unnecessary) imperative implementation of clear_lines is really a bad idea. In Python you would probably use generators to avoid loading the complete file in memory. –  tokland Nov 5 '12 at 11:47
    
What happens when the input file is larger than available memory? –  Blrfl Nov 5 '12 at 12:21
    
@Blrfl: well, if the term generator is consistent between c#/f#/python, then what @tokland and @Lattyware translates into c#/f# yield and/or yield! statements. It's a bit more obvious in my f# example because Seq.filter can only be applied to collections of IEnumerable<T> but both code examples will work if lines is a generated collection. –  Steve Evers Nov 5 '12 at 15:12

I recently had to implement a firmware programmer using the Motorola S-record format, very similar to what you describe. Since we had some time pressure, my first draft ignored redundancies and made simplifications based on the subset I actually needed to use in my application. It passed my tests easily, but failed hard as soon as someone else tried it. There was no clue what the problem was. It got all the way through but failed at the end.

So I had no choice but to implement all the redundant checks, in order to narrow down where the issue was. After that, it took me around two seconds to find the issue.

It took me maybe two hours extra to do it the right way, but wasted a day of other people's time as well in troubleshooting. It's very rare that a few processor cycles are worth a day of wasted troubleshooting.

That being said, where reading files are concerned, it's often beneficial to design your software to work with reading it and processing it one line at a time, rather than reading the entire file into memory and processing it in memory. That way it will still work on very large files.

share|improve this answer
    
"It's very rare that a few processor cycles are worth a day of wasted troubleshooting." Thanks for the answer, you have a good point. –  mcwise Nov 6 '12 at 16:10

You can raise an exception in else case. This way it's not redundant. Exceptions aren things that aren't supposed to happen but are checked anyway.

clear_lines() # removes every other line than those starting with "a" or "b"
for line in lines:
    if (line.startsWith("a)):
        # do stuff
    if (line.startsWith("b")):
        # magic
    else:
        throw BadLineException
# ...
share|improve this answer
    
I would argue the latter is a bad idea, as it's less explicit - if you later decide to add a "c", it could be less clear. –  Lattyware Nov 5 '12 at 2:04
    
First suggestion has merit... the second (assume "b") is a bad idea –  Andrew Nov 5 '12 at 9:37
    
@Lattyware I improved the answer. Thanks for your comments. –  user61852 Nov 5 '12 at 10:33
1  
@Andrew I improved the answer. Thanks for your comments. –  user61852 Nov 5 '12 at 10:33

In design by contract, one guesses each function must do its job as described in its documentation. So, each function has a list of pre-conditions, that is, conditions on the function's inputs as well as post-conditions, that is, conditions of the function's output.

The function must guarantee to its clients that, if the inputs respect the pre-conditions, then the output will be as described by the post-conditions. If at least one of the pre-conditions is not respected, the function can do what it wants (crash, return any result, ...). Therefore pre and post-conditions are a semantic description of the function.

Thanks to contract, a function is sure its clients use it correctly and a client is sure the function does its job correctly.

Some languages handle contracts natively or through a dedicated framework. For the others, the best is to check the pre and post-conditions thanks to asserts, as @Lattyware said. But I would not call that defensive programming, since in my mind this concept is more focused on the protection against the (human) user's inputs.

If you exploit contracts, you can avoid the redundantly checked condition since either the called function perfectly works and you don't need the double check, or the called function is dysfunctional and the calling function can behave as it wants.

The harder part is then to define which function is responsible of what, and to strictly document these roles.

share|improve this answer

You actually don't need the clear_lines() at the start. If the line is neither "a" or "b", the conditionals simply won't trigger. If you want to get rid of those lines then make the else into a clear_line(). As it stands you are doing two passes through your document. If you skip the clear_lines() at the beginning and do it as part of the foreach loop then you cut your processing time in half.

It's not only bad style, it's bad computationally.

share|improve this answer
2  
It could be that those lines are being used for something else, and they need to be dealt with before dealing with the "a"/"b" lines. Not saying it's likely (the clear name implies they are being discarded), just that there is a possibility it's needed. If that set of lines is repeatedly iterated over in the future, it could also be worthwhile to remove them beforehand to avoid a lot of pointless iteration. –  Lattyware Nov 5 '12 at 2:05

If you actually want to do anything if you find an invalid string (output debug text for example) then I'd say that's absolutely fine. A couple of extra lines and a few months down the line when it stops working for some unknown reason you can look at the output to find out why.

If, however, it's safe to just ignore it, or you know for certain you will never get an invalid string then there's no need for the extra branch.

Personally I'm always for putting in at least a trace output for any unexpected condition - it makes life much easier when you have a bug with output attached telling you exactly what went wrong.

share|improve this answer

... 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 ...

I hate if...then...else constructions. I would avoid the whole issue:

process_lines_by_first_character (lines,  
                                  'a' => { |line| ... a code ... },
                                  'b' => { |line| ... b code ... } )
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.