Code Smells & Refactoring

Have you ever encountered code that you know something's wrong with, but you can't tell exactly what? You just know it looks bad.

Or maybe you are getting started in a new team or company and you want to make sense of the codebase, or even improve it.

Refactoring is a wonderful way of starting to learn a new codebase. As you refactor and clean up the code, you will be forced to figure out the logic and you will be able to understand the architecture more deeply. One thing to consider is to refactor BEFORE adding any new complex functionality or feature. Doing it after will require you to refactor more and the complexity will only keep increasing.

How To Start the Refactoring Process?

Create Tests in order to understand what is going on.

By creating tests you will have to look for the functions of the file you are refactoring. Doing this will help you see the data coming in and out of the functions. While you write tests, you are also writing documentation. There is no better documentation than tests.

**Look for Code Smells. **

Looking for them combines applying well-established good practices and trusting your intuition (aka experience). Code smells are basically things that make the code too complex, hard to read, and just UGLY.

We have all written ugly code at some point, but dealing with other people's ugly code could be tricky. Also, there are plenty of code-smells lists out there, so I tried to compile the 7 most important ones (for me):

Code-Smells

Here is some code that really needs refactoring. You don't even need to understand what it's doing to know it needs it.

// @ts-nocheck
// calculate ride
export function calc (movArray) {
    let result = 0;
    for (const mov of movArray) {
        if (mov.dist != null && mov.dist != undefined && typeof mov.dist === "number" && mov.dist > 0) {
            if (mov.ds != null && mov.ds != undefined && mov.ds instanceof Date && mov.ds.toString() !== "Invalid Date") {

                // overnight

                if (mov.ds.getHours() >= 22 || mov.ds.getHours() <= 6) {

                    // not sunday
                    if (mov.ds.getDay() !== 0) {

                        result += mov.dist * 3.90;
                    // sunday
                    } else {
                        result += mov.dist * 5;

                    }
                } else {
                    // sunday
                    if (mov.ds.getDay() === 0) {

                        result += mov.dist * 2.9;

                    } else {
                        result += mov.dist * 2.10;

                    }
                }
            } else {
                // console.log(d);
                return -2;
            }
        } else {
            // console.log(dist);

            return -1;
        }

    }
    if (result < 10) {
        return 10;
    } else {
        return result;
    }
}

STOP. Before moving along, take a look at the code again and try to use your intuition to decide what should be changed. Now we will look at a couple of common code smells and what to do with them:

1. Strange or confusing names for variables or functions

Rename them.
Make the name make sense for the data it contains or the functionality it has.
This also applies to files, classes, etc.

2. Magic Numbers that aren't self-explanatory

Extract them into constants and make them variables with names that explain what they are.

3. Comments that are just fluff

Make statements self-explanatory by extracting the variable names or methods.
Look at step #6 for examples.

4. Dead Code

Code that is commented out and has no functionality anymore.
Delete it. It can always be recovered from the git history if they end up affecting something in a very obscure, unpredictable way (and should also be refactored if this happens).

5. Blank space/lines (for methods/functions)

Remove them for readability.
Different developers have different spacing styles. It is better just to remove it so everyone's code looks somewhat similar.
For those who prefer spaces when coding (dyslexia, reading disability), code your way but then you could use prettier/eslint to format it and remove the blank spaces when you commit.

6. Confusing conditions / Nested Conditions

Use extract methods (methods that return the logic you want to check for) so that conditions are not long
For example, if you have a long condition inside an IF statement, you could extract it to a function that returns it for you.

Long condition:

if (date.getHours() >= OVERNIGHT_START || date.getHours() <= OVERNIGHT_END) {
    //some code...
}

Extracted to a function:

function isOvernight (date) {
    return date.getHours() >= OVERNIGHT_START || date.getHours() <= OVERNIGHT_END;
}

//And then used in the statement:
if (isOvernight(segment.date)) {
    //some code...
}

Implement return statements to exit early in case of conditions that check if we are NOT going to execute the main logic. Move "else-returns" to the top of the function. These are called guard clauses. They help you exit the algorithm's logic fast so that the program doesn’t get into the “main logic” if it doesn’t need to. This will help remove nested conditions.
Remove nested conditions by consolidating conditions. This means composing conditions.
If you are inside a loop, add a "continue" statement on “if checks” so that the loop can continue onto the next iteration.

Example of nested conditions:

//this is happening inside of a loop
//we have a condition
if (isOvernight(segment.date)) {
    //we have a nested condition that could be consolidated if put above
    if (!isSunday(segment.date)) {
      fare += segment.distance * OVERNIGHT_FARE;
    } else {
        fare += segment.distance * OVERNIGHT_SUNDAY_FARE;
    }
}

After consolidation/composition && adding continue statements:

//we have removed the nested condition and added to the top
if (isOvernight(segment.date) && !isSunday(segment.date)) {
    fare += segment.distance * OVERNIGHT_FARE;
    continue; //to make the loop go to next iteration
}
//turned the else into another condition with the opposite check,
//thus removing nesting
if (isOvernight(segment.date) && isSunday(segment.date)) {
    fare += segment.distance * OVERNIGHT_SUNDAY_FARE;
    continue;
}

Use a ternary condition if you have an if/else that’s just for doing an assignment

7. Lack of expectation treatment

Throw errors instead of just returning something or some magic number that is supposed to mean something.

Code after steps 1-7:

// @ts-nocheck

const OVERNIGHT_FARE = 3.90;
const SUNDAY_FARE = 2.90;
const OVERNIGHT_SUNDAY_FARE = 5;
const NORMAL_FARE = 2.1;
const OVERNIGHT_START = 22;
const OVERNIGHT_END = 6;
const MIN_FARE = 10;

function isOvernight (date) {
    return date.getHours() >= OVERNIGHT_START || date.getHours() <= OVERNIGHT_END;
}

function isSunday (date) {
    return date.getDay() === 0;
}

function isValidDistance (distance) {
    return distance != null && distance != undefined && typeof distance === "number" && distance > 0;
}

function isValidDate (date) {
    return date != null && date != undefined && date instanceof Date && date.toString() !== "Invalid Date";
}

export function calculateRide (segments) {
    let fare = 0;
    for (const segment of segments) {
        //guarding clauses/statements
        if (!isValidDistance(segment.distance)) throw new Error("Invalid Distance");
        if (!isValidDate(segment.date)) throw new Error("Invalid Date");
        //main logic statements
        if (isOvernight(segment.date) && !isSunday(segment.date)) {
            fare += segment.distance * OVERNIGHT_FARE;
            continue;
        }
        if (isOvernight(segment.date) && isSunday(segment.date)) {
            fare += segment.distance * OVERNIGHT_SUNDAY_FARE;
            continue;
        }
        if (isSunday(segment.date)) {
            fare += segment.distance * SUNDAY_FARE;
            continue;
        }
        fare += segment.distance * NORMAL_FARE;
    }
    return (fare > MIN_FARE) ? fare : MIN_FARE;
}

Many Engineers would point out that there are more things that could be improved, like the method being too long, or that the code is still procedural and should be object-oriented. I understand those recommendations, but I am trying to keep my own recommendations as non-dogmatic as possible. Also, let's not forget that overengineering is a thing, and many "recommendations" are a matter of preference.

Conclusion

Refactoring code smells may seem daunting if it is your first time. But having clean, readable code pays dividends in the long run. Don't let technical debt accumulate. Refactoring improves understandability, reduces bugs, and makes adding new features easier. With some practice, you'll learn how to identify and eliminate code smells efficiently. Work on small sections at a time. Over time, you'll develop an intuition for which refactorings provide the most value. The results are absolutely worth the investment - both for you and for other engineers who will work on your projects in the future.

Dev Ideas by Bryan G.