Thursday, December 11, 2008

On Return Values and Readability

Recently, I’ve been doing a lot of refactoring, and the importance of variable names has been re-impressed on my mind. Even in relatively clean code, changing the names of variables can improve things dramatically. Let’s start with the common return variable named result. The following is some relatively easy to understand code that can be altered to improve readability.

public Person GetPersonById(int id)

{

    if (id < 0)

        throw new ArgumentException("Id cannot be less than 0");

 

    Person result = null;

 

    using (SqlConnection cn = new SqlConnection(Config.SqlConnectionString))

    {

        cn.Open();

        using (SqlCommand cm = cn.CreateCommand())

        {

            cm.CommandText = "Person_GetById";

            cm.CommandType = CommandType.StoredProcedure;

            using (SqlDataReader dr = cm.ExecuteReader())

            {

            if (dr.Read())

                {

                    result = new Person();

                    result.Id = dr.GetInt32(dr.GetOrdinal("Id"));

                    result.FirstName = dr.GetString(dr.GetOrdinal("FirstName"));

                    result.LastName = dr.GetString(dr.GetOrdinal("LastName"));

                }

            }

        }

    }

 

    return result;

}


Now, maybe you think I’m picking a little bit too much, but I don’t like the variable result here. It’s not that result isn’t descriptive (we obviously know throughout the code that result is going to be returned from the method), it’s that it causes a simple readability issue that could be changed. Take the last line for instance:

return result;


Although this line is straightforward and simple, it tells me absolutely nothing in and of itself about the type of result that I’m returning. In order to get that, I either have to look up a few lines at the return type of the method, or I have to look at the declaration of the variable earlier on in the code. While that doesn’t seem like a whole lot to ask of your fellow developers who are reading your code after you, using result isn’t doing them any favor when they have literally hundreds of methods that end this exact same way. Simply changing this variable name can make that single line more readable. Which looks better, the above return statement, or the following:

return person;


Obviously, the second is more descriptive. In one statement, it’s clear I’m returning a person. It’s true that I could have looked at the method declaration at the top to determine that I will be returning a person, however, that’s going to add another split second of brain power (and perhaps mouse scrolling) to glance back up to the top of the method. Multiply this several times for each method, and you'll quickly have a difficult-to-skim code base.

This leads me to my next complaint about the naming conventions in this code. There are three variables in named cn, cm, and dr respectively, and they’re all acronyms. I hate acronyms with a passion, unless they’re very well-known acronyms to the rest of the world. There’s a difference between using SQL and using cn. SQL is well-known, whereas cn is proprietary to my own mind. I might have always used cn, but another developer may not. This is not an accepted standard. Never assume that other people use the same variable naming standards and habits as you. This needs to be named something more descriptive, such as sqlConnection.

Before I show you my final code, I should tell you I decided to make one more change in the way the code is laid out. I separated the retrieval of the column ordinal numbers from the code that fetches the value of the columns. There’s two reasons for this. The first is (of course) readability. It’s easier to visually understand one line that gets the ordinal number for a column, than it is to understand one line that does that and fetches the value at the same time.

The final version looks like this:

public Person GetPersonById(int id)

{

    if (id < 0)

        throw new ArgumentException("Id cannot be less than 0");

 

    Person person = null;

 

    using (SqlConnection sqlConnection = new SqlConnection(Config.SqlConnectionString))

    {

        sqlConnection.Open();

 

        using (SqlCommand sqlCommand = sqlConnection.CreateCommand())

        {

            sqlCommand.CommandText = "Person_GetById";

            sqlCommand.CommandType = CommandType.StoredProcedure;

 

            using (SqlDataReader sqlDataReader = sqlCommand.ExecuteReader())

            {

                int idColumn = sqlDataReader.GetOrdinal("Id");

                int firstNameColumn = sqlDataReader.GetOrdinal("FirstName");

                int lastNameColumn = sqlDataReader.GetOrdinal("LastName");

 

                if (sqlDataReader.Read())

                {

                    person = new Person();

                    person.Id = sqlDataReader.GetInt32(idColumn);

                    person.FirstName = sqlDataReader.GetString(firstNameColumn);

                    person.LastName = sqlDataReader.GetString(lastNameColumn);

                }

            }

        }

    }

 

    return person;

}


Now, you may disagree with this approach. However, I believe the major advantage here is that every single line can be easily understood as a unit all to itself. I could feed the average developer any one line out of this whole block of code, and he should be able to tell me exactly what the line is doing. This makes for easy skimming and easy understandability.

Hopefully, in reading this, you’ve come to the same conclusion that I have. Naming your variables properly is of utmost importance. Here’s a couple of rules to keep in mind:

1. Don’t use result (or any of it’s variants such as rtn, returnValue, or ret. Even if the result value is a primitive type, the value should stand for something. Name your return values based on what they stand for in the business world. C# is an object-oriented language. Think in objects, and name your parameters, including your return value, appropriately.

2. Read your code aloud. Does it make sense when you read it aloud? If not, you could probably change something to make it more readable.

3. Move outside of “this-week-thinking”. We developers tend to get caught up in what we’re doing right now without concern for the future. In the excited frenzy of creativity and laying out new code, we can often forget that we’re going to have to read our own code sometime in the future, after we forget what we originally wrote. Try to remove yourself from the current situation and read the method as though you’ve never seen it before. Does it describe itself? If not, do the refactoring immediately after writing the method.

Over the next few posts, I hope to be writing more about clean code production and refactoring. I hope they help.