Pretty code: Skeptical of elses

Pretty code is readable code. One strategy for code beautification is to look critically at every if/else statement. Is there a more streamlined way to write that statement? Cultivate a general mistrust of elses.

Some examples...

Testing a boolean to return a boolean

if (x == true)
{ 
  return true;
}
else 
{
  return false;
}

becomes

return x;

If you need to swap those,

return !x;

That's a pattern. Recall that comparison operators (less than, greater than, equal to) return a boolean. So this also applies here:

if (myPropertyToCheck == someValue) 
{
  return true;
}
else
{
  return false;
}

becomes

return myPropertyToCheck == someValue;

Guard clauses
You can use a guard clause when you are testing if it is safe to do something, and if not, you want to exit.

if (safeToDoThis)
{
  DoThis();
}
else
{
  return;
}

becomes

if (!safeToDoThis) return;
DoThis();

When the DoThis() is rather involved, guard clauses greatly help the readability for your future teammates (even when that's you). Step 1, check if we're in an okay state, and if not, just get out of there. This saves you from the Hunt The Else game (although, if that matching else is that hard to find, you could break up your method into smaller pieces with more specific responsibilities).

Comparisons
Compare() and CompareTo() methods need to return -1, 1, or 0. It's handy that these are integers, because now you can harness the Power of Arithmetic to do your bidding. Also when you are comparing your own classes, you often want to compare a property that is a value type or a string. Those already have their own CompareTo() methods, which you can borrow.

Say you want to sort your children by their ages. You don't need

if (child1.age < child2.age)
{
  return -1;
}
else if... 

(Not just an if/else, but an if/elseif/else. Aaah!) Use instead:

return child1.Age.CompareTo(child2.Age);

If you want to sort them from oldest to youngest, this is where the arithmetic comes in. To flip a negative 1 into a 1, multiply it by negative 1. Same for flipping positive 1 into a negative 1. And zero, conveniently, doesn't mind being multiplied by anything. So you could say:

return -1 * child1.Age.CompareTo(child2.Age);

You can get it even simpler. Because -(-1) = 1, and -(1) = -1, your Compare() method becomes:

return -child1.Age.CompareTo(child2.Age);

Not only no elses, but also no ifs! Very pretty.

Reducing the number of paths through your code (i.e., reducing cyclomatic complexity) gets it closer to being read like prose. Simpler code has fewer bugs, and your successors who have to read your code will think you are smart and good looking. Keep a healthy mistrust of else statements, and write pretty code.

Dictionary as DropDownList Data Source

I have a Dictionary<T,T> called myListOptions. To use it as the datasource for an ASP.NET DropDownList called MyList...

MyList.DataSource = myListOptions;
MyList.DataTextField = "Value";
MyList.DataValueField = "Key";
MyList.DataBind();

It took me some rummaging to realize that you use the actual strings "Value" and "Key," so I hope this post comes in handy for someone else in the future (even if it's me).

The elegance of Queues and Stacks

I've been meditating on the beauty of Queues and Stacks. Like Lists, they are collections of objects, but with their own special attributes.

Queues are like a line of customers: the first one in line is the first one you'll serve (FIFO, First In First Out). These could be used for queued up messages that you have to process in order.

Stacks are like a deck of playing cards: the last one you lay on the top will be the first one you draw off the top (LIFO, Last In First Out). These could be used for tracking a sequence of actions that you might need to undo.

You can Peek() at the top item in a Queue or a Stack, but you can't reference a specific item by index. You have access only to the First In or Last In (respectively), and can only add things to the next spot in line.

I like how Queue.Dequeue() and Stack.Pop() both return the next object and remove it from the collection. It seems tidy to do it all in one method. But you're not stuck with that; like I said, you can still Peek() if you just want to see what's next, but leave the object in place.

If you need to deal with your objects in an order relating to how you encountered them, as opposed to sorting by some attribute of the object, then a Queue or a Stack might be a better fit than a List.

Fail Fast, and Mind the Outs

I had a learning opportunity on two coding concepts (in C#) yesterday:

• out parameters are tricksy and not to be trusted.
• Fail fast. When the world starts getting uncertain and hairy, throw an exception and get out of there.
  

The setup: I'm retrieving app configuration values from the database, a minimum and maximum allowable threshold. The db table is generic, containing key/value pairs for many purposes, so the values are stored as varchars, but I want integers here. If I don't get a value, I have some defaults we should use. The colleague who would be reviewing my code has declared he is allergic to nested if/else blocks, so I need a direct path through this logic.

Too crufty: One might be tempted to think about the logic this way. If I get a value from the database, but it isn't an int, set it to the default; if I don't get a value from the database, set it to the default. When you say it in English, you can hear the repeated code, violating the DRY principle. It's a small repetition, and in a very localized piece of code, but still icky.

Tripping over outs: Since I didn't want to repeat that default value, and I wasn't happy with those elses, I tried this. The default value is 100; if you get a different integer from the database, use that. But I didn't get it quite right:

[using a data reader]
int max = 100;
if (dataReader.Read())
{
   Int32.TryParse(dataReader.GetString(0), out max); 
     //This is wrong.
}
return max;

Can you see what I was thinking, though? "If you successfully parse the result into an integer, set 'max' to that value. Otherwise, leave it alone." That was my mistake. What actually happens is, if TryParse can't parse it into an integer, it makes it undefined. From an msdn article, "The value of an out argument will not be passed to the out parameter."

What I really needed to do there is, "if not int.tryparse, then set it to my default value." I should check the boolean return value whenever I use TryParse.

Hold on, cowboy: But what does it mean if the configuration value in my database isn't an integer? Somebody typed something screwy. And they probably did it by updating the database, not using the admin screen. The world is in an unknown state, society is breaking down—cats and dogs, living together...

I don't want to set any default value. I want to throw an exception and get the heck out of there. So the final version is:

[using a data reader]
int max = 100;
if (dataReader.Read())
{
   max = Int32.Parse(dataReader.GetString(0)); 
     //Fail fast and make your escape.
}
return max;

This achieves three design principles, which is why I wanted to write it down and reinforce my learning:

1. Find the most direct path through the logic. Elses are a smell.
2. Don't repeat yourself. Even though the repeating here was trivial, it caught my attention and made me think through what should really happen, which led me to...
3. When you start getting unexpected behavior, hiding it and swallowing it will cause worse problems. If you get a "that can't happen" scenario, throw an exception.