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 101 - Comparison Against Booleans

When comparing to booleans, we perform magic castings and get unexpected results.

TL;DR: Don't compare against true. Either you are true, or false or you shouldn't compare

Problems

Hidden castings
The least surprise principle violation.
Fail Fast principle violation

Solutions

Use booleans
Don't mix booleans with boolean castable objects

Context

Many languages cast values to boolean crossing domains.

Sample Code

Wrong

#!/bin/bash

if [ false ]; then
    echo "True"
else
    echo "False"
fi

# this evaluates to true since 
# "false" is a non-empty string


if [ false ] = true; then
    echo "True"
else
    echo "False"
fi

# this also evaluates to true

Right

#!/bin/bash

if  false ; then
    echo "True"
else
    echo "False"
fi

# this evaluates to false

Detection

[x]Automatic

Linters can check for explicit comparisons and warnings.

Conclusion

It is a common industry practice to use many non-booleans as booleans. We should be very strict when using booleans.

Relations

Code Smell 69 - Big Bang (JavaScript Ridiculous Castings)

More Info

Fail Fast

Credits

Photo by Michael Held on Unsplash

If it doesn’t work, it doesn’t matter how fast it doesn’t work.

- Mich Ravera

Code Smell 102 - Arrow Code

Nested IFs and Elses are very hard to read and test

TL;DR: Avoid nested IFs. Even Better: Avoid ALL IFs

Problems

Readability

Solutions

Extract Method
Combine Boolean Conditions
Remove accidental IFs

Context

In procedural code, it is very common to see complex nested ifs. This solution is more related to scripting than object-oriented programming.

Sample Code

Wrong

if (actualIndex < totalItems)
    {
      if (product[actualIndex].Name.Contains("arrow"))
      {
        do
        {
          if (product[actualIndex].price == null)
          {
            // handle no price
          }
          else
          {
            if (!(product[actualIndex].priceIsCurrent()))
            {
              // add price
            }
            else
            {
              if (!hasDiscount)
              {
                // handle discount
              }
              else
              {
                // etc
              }
            }
          }
          actualIndex++;
        }
        while (actualIndex < totalCounf && totalPrice < wallet.money);
      }
      else
        actualIndex++;
    }
    return actualIndex;
  }

Right

foreach (products as currentProduct)
  addPriceIfDefined(currentProduct)

addPriceIfDefined() 
{
  //Several extracts
}

Detection

[x]Automatic

Since many linters can parse trees, we can check on compile-time for nesting levels.

Conclusion

Following uncle bob's advice, we should leave the code cleaner than we found it.

Refactoring this problem is easy.

Relations

Code Smell 78 - Callback Hell

Code Smell 03 - Functions Are Too Long

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

More Info

The purpose of software engineering is to control complexity, not to create it.

- Pamela Zave

Code Smell 103 - Double Encapsulation

Calling our own accessor methods might seem a good encapsulation idea. But it is not.

TL;DR: Don't use setters and getters, even for private use

Problems

Setters
Getters
Exposing private attributes

Solutions

Remove setters
Remove getters
Protect your attributes

Context

Using double encapsulation was a standard procedure in the 90s.

We wanted to hide implementation details even for private use.

This was hiding another smell when too many functions rely on data structure and accidental implementation.

For example, we can change an object's internal representation and rely on its external protocol.

Cost/benefit is not worth it.

Sample Code

Wrong

contract MessageContract {
    string message = "Let's trade";
    
    function getMessage() public constant returns(string) {
        return message;
    }
    
    function setMessage(string newMessage) public {
        message = newMessage;
    }
    
    function sendMessage() public constant {
        this.send(this.getMessage());
        // We can access property but make a self call instead
    }
}

Right

contract MessageContract {
    string message = "Let's trade";
        
    function sendMessage() public constant {
        this.send(message);
    }
}

Detection

[x]Semi-Automatic

We can infer getters and setters and check if they are invoked from the same object.

Conclusion

Double encapsulation was a trendy idea to protect accidental implementation, but it exposed more than protected.

Relations

Code Smell 37 - Protected Attributes

Code Smell 28 - Setters

Code Smell 68 - Getters

More Info

Credits

Photo by Ray Hennessy on Unsplash

Encapsulate the concept that varies.

- Erich Gamma

Code Smell 104 - Assert True

Asserting against booleans makes error tracking more difficult.

TL;DR: Don't assert true unless you are checking a boolean

Problems

Fail Fast Principle

Solutions

Check if the boolean condition can be rewritten better
Favor assertEquals

Context

When asserting a boolean our test engines cannot help us very much.

They just tell us something failed.

Error tracking gets more difficult.

Sample Code

Wrong

<?

final class RangeUnitTest extends TestCase {
 
  function testValidOffset() {
    $range = new Range(1, 1);
    $offset = $range->offset();
    $this->assertTrue(10 == $offset);    
    // No functional essential description :(
    // Accidental description provided by tests is very bad
  }  
}

// When failing Unit framework will show us
//
// 1 Test, 1 failed
// Failing asserting true matches expected false :(
// () <-- no business description :(
//
// <Click to see difference> - Two booleans
// (and a diff comparator will show us two booleans)

Right

<?

final class RangeUnitTest extends TestCase {
 
  function testValidOffset() {
    $range = new Range(1, 1);
    $offset = $range->offset();
    $this->assertEquals(10, $offset, 'All pages must have 10 as offset');    
    // Expected value should always be first argument
    // We add a functional essential description
    // to complement accidental description provided by tests
  }  
}

// When failing Unit framework will show us
//
// 1 Test, 1 failed
// Failing asserting 0 matches expected 10
// All pages must have 10 as offset <-- business description
//
// <Click to see difference> 
// (and a diff comparator will help us and it will be a great help
// for complex objects like objects or jsons)

Detection

[x]Semi-Automatic

Some linters warn us if we are checking against boolean after setting this condition.

We need to change it to a more specific check.

Conclusion

Try to rewrite your boolean assertions and you will fix the failures much faster.

Relations

Code Smell 101 - Comparison Against Booleans

Code Smell 07 - Boolean Variables

More Info

Fail Fast

Credits

Photo by Joël de Vriend on Unsplash

I've finally learned what 'upward compatible' means. It means we get to keep all our old mistakes.

- Dennie van Tassel

Code Smell 105 - Comedian Methods

Use professional and meaningful names

TL;DR: Don't be informal or offensive

Problems

Readability
Unprofessional work

Solutions

Choose good and professional names.

Context

Our profession has a creative side.

Sometimes we get bored and try to be funny.

Sample Code

Wrong

function erradicateAndMurderAllCustomers();

// unprofessional and offensive

Right

function deleteAllCustomers();

// more declarative and professional

Detection

[x]Semi-Automatic

We can have a list of forbidden words.

We can also check them in code reviews.

Names are contextual, so it would be a difficult task for an automatic linter.

Naming conventions should be generic and should not include cultural jargon.

Conclusion

Be professional in the way you name things in your code.

Don't be trying to be a comedian by giving a variable a silly name.

You should write production code so future software developers (even you) should easily understand.

Relations

Code Smell 38 - Abstract Names

More Info

What is in a name

Credits

Photo by Stewart Munro on Unsplash

This ‘users are idiots, and are confused by functionality’ mentality of Gnome is a disease. If you think your users are idiots, only idiots will use it.

- Linus Torvalds

Software Engineering Great Quotes

And that’s all for now…

The next article will explain 5 more code smells!

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

Code smells are a classic.

Previous Code Smells

Code Smell 101 - Comparison Against Booleans

Problems

Solutions

Context

Sample Code

Wrong

Right

Detection

Tags

Conclusion

Relations

More Info

Credits

Code Smell 102 - Arrow Code

Problems

Solutions

Context

Sample Code

Wrong

Right

Detection

Tags

Conclusion

Relations

More Info

Code Smell 103 - Double Encapsulation

Problems

Solutions

Context

Sample Code

Wrong

Right

Detection

Tags

Conclusion

Relations

More Info

Credits

Code Smell 104 - Assert True

Problems

Solutions

Context

Sample Code

Wrong

Right

Detection

Tags

Conclusion

Relations

More Info

Credits

Code Smell 105 - Comedian Methods

Problems

Solutions

Context

Sample Code

Wrong

Right

Detection

Tags

Conclusion

Relations

More Info

Credits