December 18, 2012

Things That Make You Go WTF?!?

I recently discovered HatePaste, which is good, because I've been having a lot of WTF?!? moments lately with code.

Don't get me wrong, I'm all for using less-than-perfect code as an opportunity to both learn and teach about testing, refactoring, cyclomatic complexity, clarity, etc. However, there are some times when even I must shake my head and say "WTF?!?"

Here is an example of some code that precipitated one of those moments:

A single instance of code like the above might be attributable to an otherwise competent developer having a bad afternoon. But, sadly, there is nearly identical code scattered throughout the entire codebase. If I knew the developer who wrote the offending code I would have a serious code review with him/her, but it's likely that developer is no longer here (thankfully).

That thought begs the question, however, of what would I say/do in a code review with someone who apparently doesn't know that equality and inequality operators (among others) returns a boolean value? Would it be as simple as pointing out that fact, or would it require addressing more serious deficits in understanding?

How would you go about addressing code like what's above, or what's out there on HatePaste, in a code review with the developer who wrote the code?

2 comments:

Johan said...

It's not easy to make such a code review very constructive, I've done exactly such a "code review" before and it was a failure.

If I'd do it again I'd probably ask two questions
1. What type does foo == bar evaluate to?
2. Do you know what accidental complexity is?

The latter in order to initiate a discussion about it's importance in software industry.

I notice there are unusually few comments, any other helpful strategies?

Nick Viper said...

I share your pain. I've had many-a-code reviews and design reviews that went down the same path. Some people just aren't cut out for real computer science, IMO.

Another personal favorite is the following, using LINQ:

foreach(var foo in fooList)
{
var matchingBar = barList.Single(x => ...);
...
}

If you did this in TSQL, you'd probably be fired. Yet, somehow, this is acceptable in LINQ.

Another example:

foreach(var foo in fooList)
{
var myFooKey = foo.StringProperty1 + foo.StringProperty2;

var duplicatesExist = fooList.Any(x => x.StringProperty1 + x.StringProperty2 == myFooKey);
...
}