526 reads

How to Find The Stinky Parts of Your Code [Part XII]

by Maximiliano ContieriOctober 5th, 2021

Too Long; Didn't Read

There are 56 code smells that make us doubt the quality of our development. Most of these smells are just hints of something that might be wrong. We want our code to behave different on different environments, operating systems, so taking decisions at compile time is the best decision, isn't it? We need clean code and we must leave premature optimization buried in the past. We need to remove all compiler directives, model it with objects, make a serious benchmark instead of doing premature optimization. This technique was used when CPU were scarce, and nowadays, we need to leave it buried in past.

Companies Mentioned

featured image - How to Find The Stinky Parts of Your Code [Part XII]

Yet more code smells? More? Where do they come from?

We see several symptoms and situations that make us doubt the quality of our development.

Let's look at some possible solutions.

Most of these smells are just hints of something that might be wrong. They are not rigid rules.

Previous Parts

Let's continue...

Code Smell 56 - Preprocessors

We want our code to behave different on different environments, operating systems, so taking decisions at compile time is the best decision, isn't it?.

Problems

Readability
Premature Optimization
Unnecessary complexity
Debugging

Solutions

Remove all compiler directives.
If you want different behavior, model it with objects
If you think there's a performance penalty, make a serious benchmark instead of doing premature optimization.

Sample Code

Wrong

#if VERBOSE >= 2
  printf("trace message");
#endif

Right

if (runtimeEnvironment->traceDebug()){
  printf("trace message");
}

## even better with polymorphism and we avoid annoying ifs

runtimeEnvironment->traceDebug("trace message");

Detection

This is a syntactic directive promoted by several languages, therefore it is easy to detect and replace with real behavior.

Conclusion

Adding an extra layer of complexity makes debugging very difficult. This technique was used when memory and CPU were scarce. Nowadays, we need clean code, and we must leave premature optimization buried in the past.

Bjarne Stroustrup, in his book The Design and Evolution of C++, regrets on the pre-processor directives he created years before.

More info

C++ is designed to allow you to express ideas, but if you don't have ideas or don't have any clue about how to express them, C++ doesn't offer much help.

Bjarne Stroustrup

Code Smell 57 - Versioned Functions

sort, sortOld, sort20210117, workingSort, It is great to have them all. Just in case

Problems

Readability
Maintainability

Solutions

Keep just one working version of your artefact (class, method, attribute).
Leave time control to your version control system.

Sample Code

Wrong

findMatch()
findMatch_new()
findMatch_newer()
findMatch_newest()
findMatch_version2()
findMatch_old()
findMatch_working()
findMatch_for_real()
findMatch_20200229()
findMatch_thisoneisnewer()
findMatch_themostnewestone()
findMatch_thisisit()
findMatch_thisisit_for_real()

Right

findMatch()

Detection

We can add automatic rules to find versioned methods with patterns.

Like many other patters we might create an internal policy and communicate.

Conclusion

Time and code evolution management is always present in software development. Luckily nowadays we have mature tools to address this problem.

Credits

Original idea

That's why I write, because life never works except in retrospect. You can't control life, at least you can control your version.

Chuck Palahniuk

Code Smell 58 - Yo-yo Problem

Searching for a concrete method implementation? Go back and forth, up and down.

TL;DR: Don't ab(use) hierarchies.

Problems

Deep Hierarchies
Subclassification for Code Reuse
Readability
Low Cohesion
High Coupling

Solutions

Favor composition over inheritance.
Refactor deep hierarchies.

Sample Code

Wrong

<?

abstract class Controller {

}

class BaseController extends Controller {

}

class SimpleController extends BaseController {

}

class ControllerBase extends SimpleController {

}

class LoggedController extends ControllerBase {

}

class RealController extends LoggedController {

}

Right

<?

interface ControllerInterface {
  
}

abstract class Controller implements ControllerInterface {

}

final class LoggedControllerDecorator implements ControllerInterface {

}

final class RealController implements ControllerInterface {

}

Detection

Any linter can check for suspects against a max depth threshold.

Conclusion

Many novice programmers reuse code through hierarchies. This brings high coupled and low cohesive hierarchies.

Johnson and Foote established in their paper this was actually a good design recipe back in 1988. We have learned a lot from there.

We must refactor and flatten those classes.

More info

An error arises from treating object variables (instance variables) as if they were data attributes and then creating your hierarchy based on shared attributes. Always create hierarchies based on shared behaviors, side.

David West

Code Smell 59 - Basic / Do Functions

sort, doSort, basicSort, doBasicSort, primitiveSort, superBasicPrimitiveSort, who does the real work?

TL;DR: Shortcuts for mini wrappers shout for better solutions.

Problems

Readability
Bad Naming
Low Cohesion
Single Responsibility Principle

Solutions

Use good object wrappers
Use dynamic decorators

Sample Code

Wrong

<?

final class Calculator {

    private $cachedResults;

    function computeSomething() {
        if (isset($this->cachedResults)) {
            return $this->cachedResults;
        }
        $this->cachedResults = $this->logAndComputeSomething();
    }

    private function logAndComputeSomething() {
        $this->logProcessStart();
        $result = $this->basicComputeSomething();
        $this->logProcessEnd();
        return $result;
    }

    private function basicComputeSomething() {
        /// Do Real work here
    }

}

Right

<?

final class Calculator {
    function computeSomething() {
        // Do Real work here since I am Compute!
    }
}

//Clean and cohesive class, single responsibility

final class CalculatorDecoratorCache {

    private $cachedResults;
    private $decorated;

    function computeSomething() {
        if (isset($this->cachedResults)) {
            return $this->cachedResults;
        }
        $this->cachedResults = $this->decorated->computeSomething();
    }
}

final class CalculatorDecoratorLogger {

    private $decorated;

    function computeSomething() {
        $this->logProcessStart();
        $result = $this->decorated->computeSomething();
        $this->logProcessEnd();
        return $result;
    }
}

Detection

We can instruct our static linters to find wrapping methods if they follow conventions like doXXX(), basicXX() etc.

Conclusion

We came across this kind of methods some time in our developer life, We smelled something was not OK with them. Now is the time to change them!

More info

The primary disadvantage of Wrap Method is that it can lead to poor names. In the previous example, we renamed the pay method dispatchPay() just because we needed a different name for code in the original method.

Michael Feathers

Code Smell 60 - Global Classes

Classes are handy. We can call them and invoke them any time. Is this good?

TL;DR: Don't use your classes as a global point of access.

Problems

Coupling
Classes are global unless we use Namespaces.
Name polluting
Static Methods
Static Constants
Singletons

Solutions

Use namespaces, module qualifiers or similar
To avoid namespace polluting, keep the Global names as short as possible.
Class single Responsibility is to create instances.

Sample Code

Wrong

<?

final class StringUtilHelper {
    static function reformatYYYYDDMMtoYYYYMMDD($date) {
    }
}

class Singleton {

}

final class DatabaseAccessor extends Singleton {

}

Right

<?

namespace Date;

final class DateFormatter {

    function reformatYYYYDDMMtoYYYYMMDD(Date $date) {
    }
    //function is not static since class single responsibility is to create instances and not be a library of utils

}

namespace OracleDatabase;

class DatabaseAccessor {
    //Database is not a singleton and it is namespace scoped
}