Go to main page
A journey into the programming realm
The point of no return
Leaping into the unknown
Code smells refer to problems in the source code that reduce the quality of the code and that can increase technical debt or number of bugs in that area of the code.
The problem with the duplicated code is simple: The code is repeated in multiple places, and this is not good because it violates the DRY principle. A change in the duplicated code will require the developer to change in all places that used that code.
A solution for duplicated code would be to extract that part into a method and call it from all places, or use pull up field, in case the duplicated code is in classes within the same hierarchy.
In case the logic is similar but still different, we may be able to use parameterize method.
Why do we use functions at all? A reason that fits this topic is that we want to reuse the code that’s inside the function.
void function1() {
// do something
}
function2() {
// do something
}
// =>
function1() {
do_something();
}
function2() {
do_something();
}
Now we can call the do_something() function everywhere we want to have the same code executed.
The problem with long methods is that they contain many statements, loop, variables, or generally big number of lines. This is subjective, since a function with 200 lines containing a switch only with separate cases would be fine, but one in which the developer needs to keep track of multiple branches and methods called that have side effect / variables change is not. The reason for refactoring is simple, having a short method is easier to read, understand, and debug.
The following example is ok, because we can understand what the function does, although it contains 400 lines
ICommand createCommand(CommandID cmd, Data buffer) {
switch (cmd){
case CommandID.CommandOne: return new CommandOne(buffer);
case CommandID.CommandTwo: return new CommandTwo(buffer);
// 400 lines later
case CommandID.CommandN: return new CommandN(buffer);
default: return nullptr;
}
}
Red-flags that indicate that a function is too long:
Why do we want to break long methods? Just because they are too long? Just because they are harder to understand?
No, not only that!
Maybe parts of the function are useful in other places, or maybe we have duplicated code in that function.
The code will also become more maintainable by breaking the code into functions with meaningful names
As a hint, you should find small tasks that make sense to be standalone and extract them one by one, preferably from the most nested logic code.
Primitive obsession refers to using too many primitives in the code. A reason for refactoring is that we should always write for future development of the software (strive for flexibility), and most of the times, this implies adding functionality to the code.
void printUserDetails(string firstName, string lastName);
void printUserDetails(string firstName, string lastName, int age);
void printUserDetails(string firstName, string lastName, int age, string phoneNumber);
class UserDetails {
string firstName;
string lastName;
int age;
string phoneNumber;
};
void printUserDetails(UserDetails userDetails);
Most of the times, we will have to refactor it afterwards, and replace it with a class. In case this does not happen, having a big list of primitives being sent as parameters is a different type of code smell (which is literally called long number of parameters).
One problem here would be that using primitives can cause errors.
Date setDate(int day, int month, int year);
Date setDate(31, 12, 2018); // ok
Date setDate(12, 31, 2018); // also ok?
The function can verify if day is properly set (1-31), or if the year is in a realistic range, but the user can mistake the day and the month fields and the problem will still persist.
eg. setDate(01, 02, 2018) – is it 1st of February, or 2nd of January?
We can change the month to an enum, like this:
Date setDate(int day, Month month, int year);
Date setDate(31, Month::December, 2018); // ok
Date setDate(Month::December, 31, 2018); // error
Perfect, now it’s clear.
The good thing about primitives is that they can be sent over network, or we can use them in APIs without having to include the class itself.
Everyone knows what an integer is, but not everyone knows what an enum is and what are its entries, or what a class is and what are its members – unless you include it before passing data of such type.
Other problem would be that we have to validate the entries, instead of wrapping them in objects that would not be allowed if the fields were not ok.
Let’s give an example.
login(string username, string password) {
if (pasword.length() < 6) // too short
if (!password.contains(...)) // no digits, uppercase letters, etc..
}
class Password {
string _pass;
Password(string pass) {
if (pass.length() < 6) throw new PasswordLengthException(pass);
if (!password.contains(...)) throw new PasswordInvalidCombination(pass);
_pass = pass;
}
};
login(string username, Password password) {
// we know password is valid, according to our filters
}
When the code is filled with switch statements, it mostly points out to a violation of Open/Closed principle. This is because you need to modify all the switch statements when adding a new switch case, making it difficult to maintain and also problematic if some places are not updated, by mistake. As a solution, instead of switch statements, we should try to see whether we could use polymorphism there instead.
string getJobName(Worker worker) {
switch(worker.job) {
case Programmer: return "Programmer";
case Manager: return "Manager";
default: return "Unemployed";
}
}
double getJobSalaryGrade(Worker worker) {
switch(worker.job) {
case Programmer: return 1;
case Manager: return 2;
default: return -1;
}
}
If we want to add a new worker, we need to go in all functions that contain a switch and add a case for our new worker type.
But there’s a better alternative – using inheritance.
string getJobName(Worker worker) {
return worker.getJobName();
}
double getJobSalaryGrade(Worker worker) {
return worker.getJobSalaryGrade();
}
class Worker {
virtual string getJobName() = 0;
virtual double getJobSalaryGrade() = 0;
};
class Programmer : Worker {
virtual string getJobName() { return "Programmer"; }
virtual double getJobSalaryGrade() { return 1; }
};
class Manager : Worker {
virtual string getJobName() { return "Manager"; }
virtual double getJobSalaryGrade() { return 2; }
};
Now, if we want to add a new worker, we just create a class that inherits from Worker base class and overrides the virtual functions. The code will still work, and we can call the functions with the new worker type.
The problem with comments is that the compiler doesn’t read them, and the developers don’t read them either. And if they do, they are most likely out of date, and even if they are not, the developers will most of the time choose to read the code instead.
// attempt to open the file
ofstream myFile("example.txt"); // file resource
if (myFile.is_open()) {
// file is open
myFile << "Writing a line in the file";
myFile.close();
}
else {
// cannot open the file
Logger::log("Cannot open the file: example.txt");
}
A solution for fixing this code smell is to either rename the function, or extract pieces of it, in order to say what you wanted to say through the code. Adding a comment should be as a last option, when comments are indeed helpful (e.g. Client request #1234).
It is clear that those comments don’t add anything of value, so we can safely remove them.
ofstream myFile("example.txt");
if (myFile.is_open()) {
myFile << "Writing a line in the file";
myFile.close();
}
else {
Logger::log("Cannot open the file: example.txt");
}
Every class should have a single responsibility, and that responsibility should be entirely encapsulated by the class.