Eliminating Code Smells Through the Use of Simple Refactoring, Part 1

Eliminating code smells
by Nistor Marian
Nistor is a senior software developer at Tecknoworks.

I’m going to start off by explaining the concept of refactoring, why it’s useful in eliminating code smells, and when it should be applied. Then I’m going to dive into the three big types of refactoring that I’ve come across in my experience: the simple changes, the execution path changes and the design changes. In order to determine the need for refactoring we must be able identify certain code smells. Following the definition of a code smell, I will list a couple of them along with C# code examples in which I will take the reader from bad code to good code, explaining in detail how I handled the change of the code and which types of refactorings I’ve applied to achieve my goal. The smells will be split into the three categories of refactoring based on what they change.

Eliminating Code Smells

The discussion is split into three articles. In part 1, I will detail refactoring through simple changes.

Refactoring

Refactoring is one of our many tools as software craftsmen that enables us to mold unclean code into clean code. There are many types of refactoring and they can be applied either individually or in bulk in order to create code that is clean. Refactoring should be part of our daily routine of coding; whenever we code a new feature, or see some code that can be made better, we should do it. We just put our refactoring hat on and we make that code better.

Code smells

The code is a living organism. It changes and adapts rapidly and wildly in some cases, but most of the time the code begins to rot and eventually it starts to smell worse and worse. “It stinks, me thinks!” is what I like to use in order to relay the code smelliness to the colleagues on my team. Some smells are worse than others, however every smell must be removed from the code. The code has to be odourless! The code must shine!

As you uncover the code smells you also uncover the need for refactoring. The smells are signals that a code has become unclean. There are many code smells out there in the void and this is not an extensive list, by far, but it provides good coverage of some of the most encountered code smells. For each smell I will give some examples of bad code and explanations of why that code is bad. Then, where relevant, I will list the good code so you can see for yourself what was actually changed. If the resulting refactoring is more complicated, I will also include a description of the steps I’ve taken in order to get to the good code from the bad code. The code smells are categorized based on their refactoring amplitude into the three categories listed above.

Simple changes

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 something
return 4;
}
private int Method2(){
// Do something
return 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 bizarre
Method1();
// here we return the result of something that looks like a dinosaur
return Method2();
}
private int Method1(){
// Do something
return 4;
}
private int Method2(){
// Do something
return 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 something
return 4;
}
private int SomethingThatLooksLikeADinosaur(){
// Do something
return 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 something
return 4;
}
private int Method2(){
// Do something
return 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;
else
y += 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;
else
y += 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.