Customizable Navigation Bar

Sunday, December 11, 2011

(Opinion) Why Clean Code is More Important Than Efficient Code

Hey guys, this next post isn't so much of a tutorial as it is a rant on why I think you should pay more attention to the cleanliness and clarity of your code instead of the efficiency. Now don't get me wrong, efficient code is extremely important, but that can come later, and if no one is able to figure out what you did, including yourself, you are only going to create problems.

My first pointer for code clarity, is separate what you are doing. What I mean is, just because you can fit everything on one line, doesn't mean you should. First example:

let's say that there is some function that takes an object as a parameter, and you have two choices that you can enter as that parameter. One way of doing this is:

someFunction((m_someBoolean) ? m_object1 : m_object2);

Alright so basically what is happening here is that before we pass the object, we are checking to see the value of a boolean and determining which object to pass. This is all done where we normally just pass in the value. Sure it's quick and is done on one line, but it's not as clear as to what is going on compared to what we could be doing. Something that I personally try to do, is keep my code as close to English as I can, that way it's easy to read, and the code comments itself.


What we could have done instead was:

if(m_someBoolean == true)
{
        someFunction(m_object1);
}
else
{
        someFunction( m_object2);
}

The code being executed is the exact same, but it is much easier to read. Just by looking at the code we know that we only want to pass object1 if the boolean is true, and object 2 if the boolean is false. It's spread out more sure, but it also makes more sense to someone who has stumbled upon your code.

In some cases sure you are programming on your own, code clarity isn't as big of a deal, since you know what you did, and no one else has to look at it. If you are working on a team however, with multiple programmers, this is when it becomes a problem. If someone gets transferred and is now working with you, they have a heap of code that they need to go through and catch up on, and it messier and more cluttered it is, the longer it is going to take.

This next example is one that I am guilty of all of the time:

ObjectType temp;

if((temp = otherObject.GetComponent<ObjectType>()) != null)
{
        do something
}

more code

What is done here is I create a temporary variable on one line, and then set its value and check to see if it is null all within an if check. Why do I do it? Well it's usually more efficient to cache the value of an object before you do anything with it, and I also want to perform specific code if an object is there. So I create the temporary object and check to see if there is a value within the if block so everything is in one spot. At the same time this is messy.

What I could have done was:

ObjectType temp = otherObject.GetComponent<ObjectType>();

if(temp != null)
{
        do something
}

more code

This clarifies a little bit, it doesn't look as messy as before now that it's not within the if statement. Another thing we could do is:

 ObjectType temp;
 temp = otherObject.GetComponent<ObjectType>();

if(temp != null)
{
        do something
}

more code

This is as clear as it is going to get. We create our variable, then we set its value. If the value is there we do stuff, otherwise we move on.

Do you see where I am going with this? Something as simple as separating what you are doing can clear a few things up and save you time down the road when you are trying to figure out what you did way back at the beginning of your project.


My next pointer is to make your code comment itself. I said that a little bit earlier in my post and I will show you what I mean:

float f1, f2, f3;
bool b1;
Vector3 v1;

some code

if(b1)
(
        myObject.transform.position = v1 * (f1 + (f3 / f2));
}

What does that even mean? OK, so this is a little extreme, and I really hope your code doesn't look like this, but you see my point. If there is a bug, and you run into the code where the bug exists, but can't figure out what the code is doing, the code is useless, and you may as well just rewrite it.

Instead of that mess, name what your variables are. The length of the variable name does not matter since the compiler completely ignores it. So what that means is, name your variables something that you will remember. This also applies to my rule of LETTING YOUR CODE COMMENT ITSELF.

Same code, different variable names:


float m_offsetMagnitude,  m_offsetFactor, m_randomOffset;
bool m_moveObject;
Vector3 m_positionOffset;

some code

if(m_moveObject)
(
        myObject.transform.position = m_positionOffset * (m_offsetMagnitude + (m_randomOffset / m_offsetFactor));
}

This code makes a little more sense than it did before now that the names of the variables refer to what they do. Bear with me though, this example did not come out of any existing code, so what is actually happening here could be anything. My point here is, it's much easier to read the code when the names of your variables tell the user what they do instead of float1, boolean2 etc.

So to recap, and summarize what this post contains:

1) Separate and expand your code. It makes it easier to see what is going on when you have what you are doing separated on different lines. While it may take less typing and you may be able to understand it when everything is crammed in a few lines, it doesn't mean the next person will.

2) Name your variables! Name them something that makes sense! If the next person to look at your code has no idea what a few of your variables do, they will just rewrite it or sit there trying to figure it out, wasting time!

Hopefully these few simple tips (which would seem like common sense to some, but you'd be surprised), will clean up your code, and make your life, and your team's life much easier down the road.