270 reads

How to Find the Stinky Parts of Your Code [Part XXV]

by Maximiliano ContieriOctober 31st, 2022

Too Long; Didn't Read

It smells because there are likely many instances where it could be edited or improved. Most of these smells are just hints of something that might be wrong. They are not required fixed per se… (You should look into it though.) The smell of code smells is a classic, but you should look at it more carefully if you want to improve your code smell. The smell is just a hint that something is wrong, not a fix, but it's a good idea to try and improve it.

Company Mentioned

featured image - How to Find the Stinky Parts of Your Code [Part XXV]

Image by cookie_studio on Freepik

Code smells Are a Classic.

It smells because there are likely many instances where it could be edited or improved.

Most of these smells are just hints of something that might be wrong. They are not required fixed per se… (You should look into it though.)

Previous Code Smells

Let's continue...

Code Smell 121 - String Validations

You need to validate strings. So you don't need strings at all

TL;DR: Search for missing domain objects when validating strings.

Problems

Primitive obsession.
Bijection Fault
Validated strings are a subset of all possible strings.
Fail Fast principle violation.
Single Responsibility Principle violation.
DRY Principle Violation.

Solutions

Create a first-class object representing the concept under the MAPPER

Context

Serious software has lots of string validations.

Often, They are not in the correct places.

This leads to non-robust and corrupt software.

The simple solution is to build only real-world and valid abstractions.

Sample Code

Wrong

<?

// First Example: Address Validation
class Address { 
  function __construct(string $emailAddress) {
     // String validation on Address class violates SRP
     $this->validateEmail($emailAddress);
     // ...
   }
  
  private function validateEmail(string $emailAddress) {
    $regex = "/[a-zA-Z0-9_-.+]+@[a-zA-Z0-9-]+.[a-zA-Z]+/";
    // Regex is a sample / It might be wrong
    // Emails and Urls should be first class objects

    if (!preg_match($regex, $emailAddress))
    {
      throw new Exception('Invalid email address ' . emailAddress);
    }    
  }
}

// Second Example: Wordle

class Wordle { 
  function validateWord(string $wordleword) {
    // Wordle word should be a real world entity. Not a subset of strings
  }
 }

Right

<?

//First Example: Address Validation
class Address { 
  function __construct(EmailAddress $emailAddress) {
     // Email is always valid / Code is cleaner
     // ...
   }
}
  
class EmailAddress { 
  // We can reuse this object many times avoiding copy-pasting
  string $address; 
  private function __construct(string $emailAddress) {
    $regex = "/[a-zA-Z0-9_-.+]+@[a-zA-Z0-9-]+.[a-zA-Z]+/";
    // Regex is a sample / It might be wrong
    // Emails and Urls are first class objects

    if (!preg_match($regex, $emailAddress))
    {
      throw new Exception('Invalid email address ' . emailAddress);
    }   
    $this->address = $emailAddress;
  }
}

// Second Example: Wordle

class Wordle { 
  function validateWord(WordleWord $wordleword) {
    // Wordle word is a real world entity. Not a subset of string
  }
 }

class WordleWord { 
  function __construct(string $emailAddress) {
    // Avoid building invalid world words
    // For example length != 5
  }
 }

Detection

[x]Semi-Automatic

We can check all constructors validating strings and reify the missing concepts.

Conclusion

Small objects are hard to find.

Primitive obsessors always complain about this kind of indirections.

Creating these new small concepts keeps our model loyal to the bijection and ensures our models are always healthy.

Relations

Code Smell 41 - Regular Expression Abusers

Code Smell 04 - String Abusers

More Info

Credits

Photo by Brett Jordan on Unsplash

Less than 10% of the code has to do with the ostensible purpose of the system; the rest deals with input-output, data validation, data structure maintenance, and other housekeeping.

Mary Shaw

Software Engineering Great Quotes

Code Smell 122 - Primitive Obsession

Objects are there for the picking. Even the smallest ones.

TL;DR: Use small objects instead of primitive ones.

Problems

Code Duplication
Small Objects Missing
Fail Fast principle violation.
Bijection Fault
Subset violations: Emails are a subset of strings, Valid Ages are a subset of Real, Ports are a subset of Integers, etc.
We spread Logic and Behavior in many places.
Premature Optimization.

Solutions

Create Small Objects
Build missing abstractions using MAPPER
Use Value-Objects

Context

We are very lazy to create small objects.

We are also lazy to separate What and How.

We like very much to understand the internals of how things work.

We need to start thinking in a white box way and looking at the protocol and behavior of small components.

Sample Code

Wrong

// Samples borrowed with permission from
// https://towardsdev.com/why-a-host-is-not-a-string-and-a-port-is-not-an-integer-595c182d817c

var port = 8080;

var in = open("example.org", port);
var uri = urifromPort("example.org", port);
var address = addressFromPort("example.org", port);
var path = pathFromPort("example.org", port);

Right

// Samples borrowed with permission from
// https://towardsdev.com/why-a-host-is-not-a-string-and-a-port-is-not-an-integer-595c182d817c

const server = Port.parse(this, "www.kivakit.org:8080");
// Port is a smallobject with responsibilities and protocol

let in = port.open(this);
const uri = port.asUri(this);
const address = port.asInetSocketAddress();
const path = port.path(this, "/index.html");

Detection

[x]Manual

We can automate checks on constructors for small objects missing opportunities.

Conclusion

We need to transform our strings, numbers, and arrays into small objects.

Relations

Code Smell 121 - String Validations

Code Smell 04 - String Abusers

More Info

Credits

Photo by K. Mitch Hodge on Unsplash

Iteration allows us to progressively approach some goal. We can discard the steps that take us further away and prefer the steps that move us nearer. This is in essence how evolution works. It is also at the heart of how modern machine learning (ML) works.

Dave Farley

Software Engineering Great Quotes

Code Smell 123 - Mixed 'What' and 'How'

We love looking at the internal gears of the clock, but we need to start focusing on the hands.

TL;DR: Don't mess with implementation details. Be declarative. Not imperative.

Problems

Accidental Coupling
Coupling
Lack of design for change
Comments distinguish the 'how' and the 'what'.

Solutions

Separate 'What' and 'How' concerns.

Context

Separating concerns is very difficult in the software industry.

Functional software survives ages.

Implementation software brings coupling and is harder to change.

Choosing wise declarative names is a daily challenge.

Sample Code

Wrong

class Workflow {
    moveToNextTransition() {
        // We couple the business rule with the accidental implementation
        if (this.stepWork.hasPendingTasks) {
            throw new Exception('Preconditions are not met yet..');
        } else {
            this.moveToNextStep();
        }
    }
}

Right

class Workflow {
    moveToNextTransition() {
        if (!this.canWeMoveOn()) {
            throw new Exception('Preconditions are not met yet..');
        } else {
            this.moveToNextStep();
        }
    }

    canWeMoveOn() {
        // We hide accidental implementation 'the how'
        // under the 'what'
        return !this.stepWork.hasPendingTasks();
    }
}

Detection

[x]Manual

This is a semantic and naming smell.

Conclusion

We need to choose good names and add indirection layers when necessary.

Of course, premature optimizators will fight us, telling us we are wasting computational

resources and they need to know the insights we are hiding from them.

Relations

Code Smell 92 - Isolated Subclasses Names

Code Smell 05 - Comment Abusers

More Info

Credits

Photo by Josh Redd on Unsplash

The idea of this smell is here:

Code Smell 118 - Return False's comment

and here:

Twitter

We are constantly interfacing with other people's code that might not live up to our high standards and dealing with inputs that may or may not be valid. So we are taught to code defensively. We use assertions to detect bad data and check for consistency.

Andrew Hunt

Software Engineering Great Quotes

Code Smell 124 - Divergent Change

You change something in a class. You change something unrelated in the same class

TL;DR: Classes should have just one responsibility and one reason to change.

Problems

Coupling
Code Duplication
Low Cohesion
Single Responsibility Principle violation

Solutions

Extract class

Context

We create classes to fulfill responsibilities.

If an object does too much, it might change in different directions.

Sample Code

Wrong

class Webpage {
  
  renderHTML() {
    renderDocType();
    renderTitle();
    renderRssHeader();
    renderRssTitle();
    renderRssDescription();
   // ...
  }
  // HTML render can change

  renderRssDescription() {
   // ...
  }

  renderRssTitle() {
   // ...
  }

  renderRssPubDate() {
   // ...
  }
  // RSS Format might change

}

Right

class Webpage {
  
  renderHTML() {
    this.renderDocType();
    this.renderTitle();
    (new RSSFeed()).render();
    this.renderRssTitle();
    this.renderRssDescription();
   // ...
  }
  // HTML render can change
}

class RSSFeed {
  render() {
    this.renderDescription();
    this.renderTitle();
    this.renderPubDate();
    // ...
  }  
  // RSS Format might change
  // Might have unitary tests
  // etc
}

Detection

[x]Semi Automatic

We can automatically detect large classes or track changes.

Conclusion

Classes must follow the Single Responsibility Principle and have just one reason to change.

If they evolve in different ways, they are doing too much.

Relations

Code Smell 34 - Too Many Attributes

Code Smell 94 - Too Many imports

Code Smell 14 - God Objects

More Info

Refactoring.guru

A design that doesn’t take change into account risks major redesign in the future.

Erich Gamma

Software Engineering Great Quotes

Code Smell 125 - 'IS-A' Relationship

We learned at school that inheritance represents an 'is-a' relationship. It is not.

TL;DR: Think about protocol and behavior, forget inheritance

Problems

Bad models
Unexpected behavior
Subclass overrides
Liskov substitution principle Violation

Solutions

Think in terms of behavior behaves-as-a
Prefer composition over inheritance
Subclassify always following the 'behaves-as-a' relation

Context

IS-A relation comes from the data world.

We learned ERDs with structured design and data modeling.

Now, we need to think in terms of behavior.

Behavior is essential, data is accidental.

Sample Code

Wrong

// If you made Square derive from Rectangle, 
// then a Square should be usable anywhere you expect a rectangle

#include <iostream>

Rectangle::Rectangle(const unsigned width, const unsigned height):
    m_width(width),
    m_height(height)
{
}

unsigned Rectangle::getWidth() const
{
    return m_width;
}

void Rectangle::setWidth(const unsigned width)
{
  /* Width and Height are independent */
    m_width = width;
}

unsigned Rectangle::getHeight() const
{
    return m_height;
}

void Rectangle::setHeight(const unsigned height)
{
    m_height = height;
}

unsigned Rectangle::area() const
{
  /*Valid for both Rectangles and Squares*/
    return m_height * m_width;
}

Square::Square(const unsigned size)
    : Rectangle(size, size)
{
}

// OK for squares, bad for rectangles
// Protocol is bad, width and height are not relevant on squares
void Square::setWidth(const unsigned size)
{
    m_height = size;
    m_width = size;
}

// OK for squares, bad for rectangles
// Protocol is bad, width and height are not relevant on squares
void Square::setHeight(const unsigned size)
{
    m_height = size;
    m_width = size;
}

void process(Rectangle& r)
{
    unsigned h = 10;
    auto w = r.getWidth();
    r.setHeight(h);

    std::cout << "Expected area: " << (w*h) << ", got " << r.area() << "\n";
    // area is not well defined in squares
    // every square IS-A rectangle, but does not behave-like a rectangle
}

int main()
{
    Rectangle rectangle{3,4};
    Square square{5};
    process(rectangle);
    process(square);
}

Right

// If you made Square derive from Rectangle, 
// then a Square should be usable anywhere you expect a rectangle

#include <iostream>

Rectangle::Rectangle(const unsigned width, const unsigned height):
    m_width(width),
    m_height(height)
{
}

void Rectangle:changeWidth(const unsigned width)
{
  /* Width and Height are independant */
    m_width = width;
    // We should discuss if it is good to mutate 
    // and not create a new Figure
}

void Rectangle::changeHeight(const unsigned height)
{
    m_height = height;
}

unsigned Rectangle::area() const
{ 
    return m_height * m_width;
}

// No inheritance
Square::Square(const unsigned size):
    m_size(size)
{
}

unsigned Square::area() const
{ 
    return m_size * m_size;
}
 
void Square::changeSize(const unsigned size)
{
    m_size = size; 
}
 
void testRectangleChange(Rectangle& r)
{
    unsigned h = 10;
    auto w = r.getWidth();
    r.setHeight(h);

    std::cout << "Expected area: " << (w*h) << ", got " << r.area() << "\n";
    // area is not well defined in squares
    // every square IS-A rectangle, but does not behave-like a rectangle
}

int main()
{
    Rectangle rectangle{3,4};
    Square square{5};
    testRectangleChange(rectangle);
    testRectangleChange(square);
}