Eliminating Code Smells Through Refactoring, Part 1 - Tecknoworks

Simple changes do not affect the execution of the software system. These include renames, code removal, instructions compressions, etc. They usually only touch the look of the code and not the way the code gets executed. As a rule of thumb, whenever you do this kind of refactoring you don’t really need unit tests or any other kind of mechanism to let you know you didn’t break something.

Unused method/variable

We should remove it without any issues or worries that something might go wrong.

Bad Code

public void MainMethod(){Method1();Method2();//Method3();}private void Method1(){// Do something}private void Method2(){// Do something}public void Method3(){// Do some processing

}

The method was clearly used at some point, but instead of it being removed, it was left inside the code base. This kind of practice is bad, especially because, in time, you might end up with more unused code than actual code. Nowadays with the power that the source control systems give us, we should avoid keeping old code, or code that is not used, at all costs. Let the source control system do its magic.

Bad Code

public int MainMethod(){var x = 0;x += Method1();return Method2();}private int Method1(){// Do somethingreturn 4;}private int Method2(){// Do somethingreturn 5;}

It is obvious that the x variable is not used for anything purposely. Perhaps the old code was reading something like x += Method2(); return x;. You can only guess the reason for its existence! This is a very sloppy way of changing the MainMethod to only return the value of the Method2.

Unnecessary comments

Comments that either provide no interesting information or that no longer belong to the code should be removed. As a rule of thumb the code should be able to speak for itself, it should be clear and to the point, making all comments unnecessary. The mere fact you need comments is a good indicator that the code is not clean.

Bad Code

public int MainMethod(){// here we do something extremely bizarreMethod1();// here we return the result of something that looks like a dinosaurreturn Method2();}private int Method1(){// Do somethingreturn 4;}private int Method2(){// Do somethingreturn 5;}

Right from the very top the MainMethod contains comments that try to explain what the methods below them do. The code does not speak for itself, because in this case the name of the methods should state exactly what the comments are stating, making the comments useless.

Good Code

public int MainMethod(){DoSomethingExtremelyBizarre();return SomethingThatLooksLikeADinosaur();}private int DoSomethingExtremelyBizzare(){// Do somethingreturn 4;}private int SomethingThatLooksLikeADinosaur(){// Do somethingreturn 5;}

Arbitrary indentation of code lines

We simply need to use the IDE capabilities to sort out and align all of the lines in order to make the code easier to read and understand.

Bad Code

public int MainMethod(){Method1(); return Method2();}private int Method1(){// Do somethingreturn 4;}private int Method2(){// Do somethingreturn 5;}

If I take one look at this code my first question is, Where does Method1 end? Where does Method2 start? The code doesn’t allow me to easily distinguish this important information right away. Instead I have to scout for the end curly braces myself. Nowadays most IDEs have a simple key combination that allow you to correctly indent all the file at once. In Visual Studio for example: Ctrl+K, Ctrl+D.

Magic numbers

They should be extracted into constants that have correct names and place in the code. Having a magic number in code is usually a very rude way of saying to the person that will maintain the code that he should not touch that code at all.

Bad Code

public int MainMethod(){Method1();return Method2();}private int Method1(){return 4;}private int Method2(){return 5;}

As I look at this code I have no idea what 4 and 5 actually mean or why are they there. I can try to guess but the code gives me no clue. However, if I replace these number with constants that have a name, I can easily convey my message to the code reader about what does 4 or 5 mean.

Good Code

private const int NumberOfDolphins = 4;private const int NumberOfWhales = 5;public int MainMethod(){Method1();return Method2();}private int Method1(){return NumberOfDolphins;}private int Method2(){return NumberOfWhales;}

Naming conventions being broken

Either change the name to something that is much more perceptible from a human stand point of view, or remove the need for that class, variable or method.

Bad Code

private const int NOD = 4;private const int NOW = 5;public int MainMethod(){return Method1() + Method2();}private int Method1(){return NOD;}private int Method2(){return NOW;}

What does NOD mean? What does NOW mean? Is it referring to the actual time or what? What does Method1 do? What does Method2 do? I can’t tell. The function names are not conveying any message in regard to what they actually accomplish. The constant names do not represent eligible meanings.

Good Code

private const int NumberOfDolphins = 4;private const int NumberOfWhales = 5;public int GetTotalAquaticInhabitants(){return GetNumberOfDolphins() + GetNumberOfWhales();}private int GetNumberOfDolphins(){return NumberOfDolphins;}private int GetNumberOfWhales(){return NumberOfWhales;}

Now I can clearly understand what this code does without actually having to search for specs in regard to the functionality. I can read it like prose. If I need to change the number of dolphins, I know exactly what constant I need to set. In case we have 4 tanks, each of them holding 4 dolphins I can easily change the GetNumberOfDolphins method in order to relay this information correctly.

Unneeded code

Classic example of an if statement for which the condition has become always true or false. You no longer have use of the else and therefore of the if statement at all. This tends to happen in very complex software but occasionally it appears also in some simple projects. Just remove the code.

Bad Code

public int MainMethod(){return Method1() + Method2(false);}private int Method1(){int x = 0;for (int i = 0; i < 100; i++){bool downstream = true; // Due to change request #123 this is always true! i < 50;if (downstream){x -= i % 10;// do some more processing}else{x += i % 10;// do some more processing}}return x;}private int Method2(bool downstream){int y = downstream ? 100 : 0;return y;}

As we can see the if in Method1 is no longer useful, it was even commented to point out that it will always be true. Apparently the developer thought this code might be useful in the future, but that is rarely the case. We simply remove the unneeded code.

Good Code

public int MainMethod(){return Method1() + Method2(false);}private int Method1(){int x = 0;for (int i = 0; i < 100; i++){x -= i % 10;// do some more processing}return x;}private int Method2(bool downstream){int y = downstream ? 100 : 0;return y;}

Same rule can be applied to Method2, if we can make sure that it is never called with different Boolean values for the downstream parameter. Another good example for unneeded code are methods that are no longer part of the execution path of the software system.

Bad Code

public int MainMethod(){return Method2(false);}private int Method1(){int x = 0;for (int i = 0; i < 100; i++){x -= i % 10;// do some more processing}return x;}private int Method2(bool downstream){int y = downstream ? 100 : 0;for (int j = 0; j < 100; j++){if (downstream)y -= j % 5;elsey += j % 5;}return y;}

As we can see the Method1 is never called throughout the execution of the program. So we can safely remove it.

Good Code

public int MainMethod(){return Method2(false);}private int Method2(bool downstream){int y = downstream ? 100 : 0;for (int j = 0; j < 100; j++){if (downstream)y -= j % 5;elsey += j % 5;}return y;}

There is no reason to have code that is no longer used or that never runs, ever! Next time, we’ll talk about refactoring through execution path changes. And in the meantime, if you’d like us to take a look at cleaning up your code, please be in touch.