783 reads

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

by Maximiliano ContieriAugust 29th, 2022

Too Long; Didn't Read

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

Companies Mentioned

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

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 106 - Production Dependent Code

Don't add IFs checking for the production environment.

TL;DR: Avoid adding conditionals related to production

Problems

Fail fast principle violation
Lack of testability

Solutions

If completely necessary, model environments and test ALL of them.

Context

Sometimes, We need to create different behaviors in development and production.

For example the strength of the passwords.

In this case, we need to configure the environment with the strength strategy and test the strategy and not the environment itself.

Sample Code

Wrong

def send_welcome_email(email_address, environment):
  if ENVIRONMENT_NAME == "production":
    print(f"Sending welcome email to {email_address} from Bob Builder <[email protected]>")
  else:
    print("Emails are sent only on production")
    
send_welcome_email("[email protected]", "development")
# Emails are sent only on production

send_welcome_email("[email protected]", "production")
# Sending welcome email to [email protected] from Bob Builder <[email protected]>

Right

class ProductionEnvironment:
  FROM_EMAIL = "Bob Builder <[email protected]>"

class DevelopmentEnvironment:
  FROM_EMAIL = "Bob Builder Development <[email protected]>"
  
# We can unit test environments
# and even implement different sending mechanisms

def send_welcome_email(email_address, environment):
  print(f"Sending welcome email to {email_address} from {environment.FROM_EMAIL}")
  # We can delegate into a fake sender (and possible logger)
  # and unit test it

send_welcome_email("[email protected]", DevelopmentEnvironment())
# Sending welcome email to [email protected] from Bob Builder Development <[email protected]>

send_welcome_email("[email protected]", ProductionEnvironment())
# Sending welcome email to [email protected] from Bob Builder <[email protected]>

Detection

[x]Manual

This is a design smell.

We need to create empty development/production configurations and delegate them with customizable polymorphic objects.

Conclusion

Avoid adding untestable conditionals.

Create configurations delegating business rules.

Use abstractions, protocol, and interfaces, avoid hard hierarchies.

Relations

Code Smell 56 - Preprocessors

More Info

How to avoid annoying ifs

Credits

Photo by Birmingham Museums Trust on Unsplash

This tweet was inspired by @Jan Giacomelli

Twitter

Complexity is a sign of technical immaturity. Simplicity of use is the real sign of a well design product whether it is an ATM or a Patriot missile.

Daniel T. Ling

Software Engineering Great Quotes

Code Smell 107 - Variables Reuse

Reusing variables makes scopes and boundaries harder to follow

TL;DR: Don't read and write the same variable for different purposes

Problems

Readability
Hidden problems

Solutions

Don't reuse variables
Extract Method to isolate scopes

Context

When programming a script it is common to reuse variables.

This leads to confusion and makes debugging harder.

We should narrow the scope as much as possible.

Sample Code

Wrong

// print line total
double total = item.getPrice() * item.getQuantity();
System.out.println("Line total: " + total );

// print amount total 
total = order.getTotal() - order.getDiscount();
System.out.println( "Amount due: " + total );

// variable is reused

Right

function printLineTotal() {
  double total = item.getPrice() * item.getQuantity();
  System.out.println("Line total: " + total );
}

function printAmountTotal() {
  double total = order.getTotal() - order.getDiscount();
  System.out.println( "Amount due: " + total );
}

Detection

[ ]Automatic

Linters can use the parse tree to find variable definition and usages.

Conclusion

Avoid reusing variable names. Use more specific and different names.

Relations

Code Smell 03 - Functions Are Too Long

More Info

Refactoring 002 - Extract Method

Credits

Photo by Sigmund on Unsplash

Simplicity before generality, use before reuse.

Kevlin Henney

Software Engineering Great Quotes

Code Smell 108 - Float Assertions

Asserting two float numbers are the same is a very difficult problem

TL;DR: Don't compare floats

Problems

Wrong test results
Fragile tests
Fail fast principle violation

Solutions

Avoid floats unless you have REAL performance concerns
Use arbitrary precision numbers
If you need to compare floats compare with tolerance.

Context

Comparing float numbers is an old computer science problem.

The usual solution is to use threshold comparisons.

We recommend avoiding floats at all and trying to use infinite precision numbers.

Sample Code

Wrong

Assert.assertEquals(0.0012f, 0.0012f); // Deprecated
Assert.assertTrue(0.0012f == 0.0012f); // Not JUnit - Smell

Right

Assert.assertEquals(0.0012f, 0.0014f, 0.0002); // true
Assert.assertEquals(0.0012f, 0.0014f, 0.0001); // false
// last parameter is the delta threshold

Assert.assertEquals(12 / 10000, 12 / 10000); // true
Assert.assertEquals(12 / 10000, 14 / 10000); // false

Detection

[ ]Automatic

We can add a check con assertEquals() on our testing frameworks to avoid checking for floats.

Conclusion

We should always avoid comparing floats.

Relations

Code Smell 71 - Magic Floats Disguised as Decimals

More Info

Fail fast

Credits

Photo by Mika Baumeister on Unsplash

God made the natural numbers; all else is the work of man.

Leopold Kronecker

Software Engineering Great Quotes

Code Smell 109 - Automatic Properties

What happens if you combine 4 code smells?

TL;DR: Avoid Getters, Avoid Setters, Avoid Metaprogramming. Think about Behavior.

Problems

Information Hiding Violation
Mutability
Fail Fast Principle violation
Duplicate code when setting properties

Solutions

Remove automatic setters and getters

Context

Setters and getters are a bad industry practice.

Many IDEs favor this code smell.

Some languages provide explicit support to build anemic models and DTOs.

Sample Code

Wrong

class Person
{
  public string name 
  { get; set; }
}

Right

class Person
{
  private string name  
  
  public Person(string personName)
  {
    name = personName;
    //imutable
    //no getters, no setters
  }

  //... more protocol, probably accessing private variable name
}

Detection

[ ]Automatic

This is a language feature.

We should avoid immature languages or forbid their worst practices.

Conclusion

We need to think carefully before exposing our properties.

The first step is to stop thinking about properties and focus solely on behavior.

Relations

Code Smell 28 - Setters

Code Smell 68 - Getters

Code Smell 70 - Anemic Model Generators

Code Smell 40 - DTOs

Code Smell 01 - Anemic Models

More Info

Credits

Photo by Kony on Unsplash

Nothing is harder than working under a tight deadline and still taking the time to clean up as you go.

Kent Beck

Software Engineering Great Quotes

Code Smell 110 - Switches With Defaults

Default means 'everything we don't know yet'. We cannot foresee the future.

TL;DR: Don't add a default clause to your cases. Change it for an exception. Be Explicit.

Problems

Coupling
Fail Fast principle violation
Open closed principle violation

Solutions

Replace if and cases with polymorphism
Change Default code to an Exception

Context

When using cases, we usually add a default case so it doesn't fail.

Failing is always better than taking decisions without evidence.

Since case and switches are also an smell, we can avoid them.

Sample Code

Wrong

switch (value) {
  case value1:
    // if value1 matches the following will be executed..
    doSomething();
    break;
  case value2:
    // if value2 matches the following will be executed..
    doSomethingElse();
    break;
  default:
    // if value does not presently match the above values
    // or future values
    // the following will be executed
    doSomethingSpecial();
    break;
}

Right

switch (value) {
  case value1:
    // if value1 matches the following will be executed..
    doSomething();
    break;
  case value2:
    // if value2 matches the following will be executed..
    doSomethingElse();
    break;
  case value3:
  case value4:
    // We currently know these options exist
    doSomethingSpecial();
    break;
  default:
    // if value does not match the above values we need to take a decision
    throw new Exception('Unexpected case ' + value + ' we need to consider it');
    break;
}

Detection

[x]Semi Automatic

We can tell our linters to warn us on default uses unless there's an exception.

Conclusion

Writing robust code doesn't mean we need to take decisions without evidence.

Relations

Code Smell 36 - Switch/case/elseif/else/if statements

More Info

Fail Fast

Credits

Photo by Joshua Woroniecki on Unsplash

The cost of adding a feature isn’t just the time it takes to code it. The cost also includes the addition of an obstacle to future expansion. The trick is to pick the features that don’t fight each other.

John Carmack

Software Engineering Great Quotes

And that’s all for now…

The next article will explain 5 more code smells!