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

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

tldt arrow
Read on Terminal Reader

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

Mention Thumbnail
featured image - How to Find the Stinky Parts of Your Code [Part XXV]
Maximiliano Contieri HackerNoon profile picture

@mcsee

Maximiliano Contieri

I’m senior software engineer specialized in declarative designs and S.O.L.I.D....

About @mcsee
LEARN MORE ABOUT @MCSEE'S EXPERTISE AND PLACE ON THE INTERNET.
react to story with heart

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

image

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

  1. 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_-.+][email protected][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_-.+][email protected][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.

Tags

  • Primitive Obsession

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


image

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

  1. Create Small Objects
  2. Build missing abstractions using MAPPER
  3. 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.

Tags

  • Primitive Obsession

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'


image

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

  1. 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.

Tags

  • Readability

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


image

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

  1. 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.

Tags

  • Coupling

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


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


image


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

  1. Think in terms of behavior behaves-as-a
  2. Prefer composition over inheritance
  3. 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);
}

Detection

  • [x]Manual

This is a semantic smell.

Tags

  • Inheritance

Conclusion

Real Number IS-A Complex number (according to math).

Integer IS-A Real number (according to math).

Real Number does not Behave-Like-A Complex number.

We cannot do real.setImaginaryPart() so it is not a Complex according to our Bijection

Relations

Code Smell 92 - Isolated Subclasses Names

Code Smell 11 - Subclassification for Code Reuse

Code Smell 37 - Protected Attributes

More Info

Credits

Photo by Joshua Rondeau on Unsplash


DRY - Don't Repeat Yourself - Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

Andy Hunt

Software Engineering Great Quotes


And that’s all for now…

The next article will explain 5 more code smells!

RELATED STORIES

L O A D I N G
. . . comments & more!
Hackernoon hq - po box 2206, edwards, colorado 81632, usa