471 reads

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

by Maximiliano ContieriMay 23rd, 2022

Too Long; Didn't Read

The code smells are a classic. Most of these smells are just hints of something that might be wrong.

People Mentioned

Company Mentioned

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

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 91 - Test Asserts without Description

We are big fans of xUnit. But we don't care much for the programmers

TL;DR: Use asserts with declarative descriptions.

Problems

Readability
Hard debugging
Time waste

Solutions

Put a nice descriptive assertion
Share guides for problem-solving

Sample Code

Wrong

<?

public function testNoNewStarsAppeared(): void
  {
     $expectedStars = $this->historicStarsOnFrame();
     $observedStars = $this->starsFromObservation();
     //These sentences get a very large collection
  
     $this->assertEquals($expectedStars, $observedStars);
     //If something fails we will have a very hard debugging time
    }

Right

<?

public function testNoNewStarsAppeared(): void
  {
     $expectedStars = $this->historicStarsOnFrame();
     $observedStars = $this->starsFromObservation();
     //These sentences get a very large collection
  
     $newStars = array_diff($expectedStars, $observedStars);
  
     $this->assertEquals($expectedStars, $observedStars ,
         'There are new stars ' . print_r($newStars,true));
     //Now we can see EXACTLY why the assertion failed with a clear and
     //Declarative Message 
    }

Detection

Since assert and assertDescription are different functions, we can adjust our policies to favour the latter.

Conclusion

Be respectful to the reader of your assertions.

It might even be yourself!

More Info

XUnit: Assert Description Deprecation

Credits

Photo by Startaê Team on Unsplash

Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.

John Woods

Code Smell 92 - Isolated Subclasses Names

If your classes are globals, use fully qualified names

TL;DR: Don't use abbreviations in subclasses

Problems

Readability
Mistakes

Solutions

Rename your classes to provide context
Use modules, namespaces or fully qualified names

Sample Code

Wrong

abstract class PerserveranceDirection { 
}

class North extends PerserveranceDirection {}
class East extends PerserveranceDirection {}
class West extends PerserveranceDirection {}
class South extends PerserveranceDirection {}

//Subclasses have short names and meaningless outside the hierarchy
//If we reference East we might mistake it for the Cardinal Point

Right

abstract class PerserveranceDirection { 
}

class PerserveranceDirectionNorth extends PerserveranceDirection {}
class PerserveranceDirectionEast extends PerserveranceDirection {}
class PerserveranceDirectionWest extends PerserveranceDirection {}
class PerserveranceDirectionSouth extends PerserveranceDirection {}

//Subclasses have fully quallified names

Detection

Automatic detection is not an easy task. We could enforce local naming policies for subclasses.

Conclusion

Choose your names wisely.

If your language supports it, use modules, namespaces and local scopes.

Relations

Code Smell 11 - Subclassification for Code Reuse

More Info

Credits

Photo by Edvard Alexander Rølvaag on Unsplash

The programmer's primary weapon in the never-ending battle against slow system is to change the intramodular structure. Our first response should be to reorganize the modules' data structures.

Frederick P. Brooks

Code Smell 93 - Send me Anything

Magic functions that can receive a lot of different (and not polymorphic arguments)

TL;DR: Create a clear contract. Expect just one protocol.

Problems

Fail Fast principle violation
Error prune
Readability
If polluting
Nulls
Bad Cohesion

Solutions

Take just one "kind" of input
Arguments should adhere to a single protocol.

Sample Code

Wrong

<?

function parseArguments($arguments) {
    $arguments = $arguments ?: null;
    //Always the billion-dollar mistake
    if (is_empty($arguments)) {
        $this->arguments = http_build_query($_REQUEST);
        //Global coupling and side effects
    } elseif (is_array($arguments)) {
        $this->arguments = http_build_query($arguments);
    } elseif (!$arguments) { //null unmasked
        $this->arguments = null;
    } else {
        $this->arguments = (string)$arguments;
    }
}

Right

<?

function parseArguments(array $arguments) {
    $this->arguments = $arguments;
    //much cleaner, isn't it ?
}

Detection

We can detect this kind of methods when they do different things, asking for the argument kind

Conclusion

Magic castings and flexibility have a price. They put the rubbish under the rug and violate fail fast principle.

Relations

Code Smell 69 - Big Bang (JavaScript Ridiculous Castings)

Code Smell 06 - Too Clever Programmer

Code Smell 12 - Null

Credits

Photo by Hennie Stander on Unsplash

Referential transparency is a very desirable property: it implies that functions consistently yield the same results given the same input, irrespective of where and when they are invoked.

Edward Garson

Code Smell 94 - Too Many imports

If your class relies on too many others, it will be coupled and fragile. A long import list is a good indicator.

TL;DR: Don't import too much.

Problems

Coupling
Single Responsibility Principle violation
Low Cohesion

Solutions

Break the class
Hide intermediate accidental implementation

Sample Code

Wrong

import java.util.LinkedList;
import java.persistence;
import java.util.ConcurrentModificationException;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.NoSuchElementException 
import java.util.Queue;
import org.fermi.common.util.ClassUtil;
import org.fermi.Data;
//We rely on too many libraries

public class Demo {
   public static void main(String[] args) {
      
   }
}

Right


import org.fermi.domainModel;
import org.fermi.workflow;

//We rely on few libraries
//and we hide their implementation
//So maybe transitive imports are the same
//but we don't break encapsulation

public class Demo {
   public static void main(String[] args) {
      
   }
}

Detection

We can set a warning threshold on our linters.

Conclusion

We need to think about dependencies when building our solutions to minimize Ripple Effect.

Relations

Code Smell 61 - Coupling to Classes

More Info

Credits

Photo by Zdeněk Macháček on Unsplash

Twitter

Fools ignore complexity. Pragmatists suffer it. Some can avoid it. Geniuses remove it.

Alan Perlis

Code Smell 95 - Premature Classification

We are over generalizers. We shouldn't create abstractions until we see enough concretions.

TL;DR: Don't guess what the future will bring you.

Context

Aristotelian Classification is a big problem in computer science. We tend to classify and name things before gathering enough knowledge and context.

Problems

Futurology
Bad designs

Solutions

Wait for concretions
Refactor late

Sample Code

Wrong

class Rectangle 
 { 
       int length; 
       int breadth; 
       
       int area() 
       {
         return length * breadth;
       } 
 } 
//We are creating a premature abstraction 
//And misusing is-a relation since a Square "is a" Rectangle

class Square extends Rectangle
 { 
       int length;  
       
       int area() 
       {  
         return length * length; 
       } 
 }

Right

class Rectangle 
 { 
       int length; 
       int breadth; 
       
       int area() 
       {
         return length * breadth;
       } 
 }  

class Square 
{ 
       int length;  
       
       int area() 
       {  
         return length * length; 
       } 
 } 
//Square might-be a Rectangle
//But it does not follow behaves-like relation so we won't go ahead
//and create a strong relation between them
//Maybe they are shapes. We don't have enough examples and protocol yet
//We will not guess until further knowledge

Detection

An abstract class with just one subclass is an indicator of premature classification

Conclusion

When working with classes, we name abstractions as soon as they appear.

Our rule is to choose good names after the behaviour.

We should not name our abstractions until we name our concrete subclasses.

Relations

Code Smell 11 - Subclassification for Code Reuse

More Info

What is in a name

Credits

Photo by Faye Cornish on Unsplash

Let us change our traditional attitude to the construction of programs: Instead of imagining that our main task is to instruct a computer what to do, let us concentrate rather on explaining to human beings what we want a computer to do.

Donald E. Knuth

Software Engineering Great Quotes

And that’s all for now…

The next article will explain 5 more code smells!