Pet Peeve Code Smell: Multiple Calls to a Method in a Block

I was just reading Ayende’s post on coding pitfalls and it’s associated comments. This struck me as less about pitfalls and more about pet peeves. A personal pet peeve of mine is when I discover a block of code that makes multiple calls out to fetch a single value while processing.

For example:

if(Call.ToSomethingByName("Name") != "foo") {
  if(Call.ToSomethingByName("Name") !="bar")
    var something = Call.ToSomethingByName("Name");
    processNonFoos(something);
  else
    var someotherthing = Call.ToSomethingByName("Name");
    processFoos(something);
}

Just seeing this kind of construct tells me the person who wrote it was not thinking about anything other than “make it work”. They were so focused on getting the task at hand done and moving on they didn’t stop to think about the all the possible gotchas and potential failure cases.

This code was most likely tested by hitting F5, plugging some text into a form and clicking submit. As long as no errors were thrown the developer moves on to the next task.

Just looking at this code, how many possible failure points do you see? What happens if Call.ToSomethingByName(“Name”) return null? What happens if Call.ToSomethingByName(“Name”) makes calls to the database? What happens if Call.ToSomethingByName(“Name”) returns something different on each call?