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 111 - Modifying Collections While Traversing

Changing a collection while traversing might lead to unexpected errors

TL;DR: Do not modify collections while traversing them

Problems

• Unexpected Results
• Concurrency problems

Solutions

1. Avoid altering the collections
2. Make collection copies

Context

We over-optimize our solutions with the prejudice that copying collections are expensive.

This is not true for small and medium-size collections.

Languages iterate collections in many different ways.

Modifying them is generally not safe.

Sample Code

Wrong

Collection<Integer> people = new ArrayList<>();
// here we add elements to the collection...
  
for (Object person : people) {
    if (condition(person)) {
        people.remove(person);
    }
}
// We iterate AND remove elements

Right

Collection<Integer> people = new ArrayList<>();
// Here we add elements to the collection...

List<Object> iterationPeople = ImmutableList.copyOf(people);
    
for (Object person : iterationPeople) {
    if (condition(person)) {
        people.remove(person);
    }
}
// We iterate a copy and remove it from original

coll.removeIf(currentIndex -> currentIndex == 5);
// Or use language tools (if available)

Detection

• [x]Semi Automatic

Many languages provide control both in compile and run-time

• Fail Fast

Conclusion

This is something we learn in our first courses.

It happens a lot in the industry and real-world software

Relations

Code Smell 53 - Explicit Iteration

More Info

• Stack Overflow

Credits

Photo by Christine Roy on Unsplash

---

Bugs are bugs. You write code with bugs because you do. If it’s a safe language in the sense of run-time safe, the operating system crashes instead of doing a buffer overflow in a way that’s exploitable.

- Ken Thompson

Software Engineering Great Quotes

---

Code Smell 112 - Testing Private Methods

If you work on unit testing, sooner or later you will face this dilemma

TL;DR: Don't test your private methods.

Problems

• Breaking Encapsulation
• Code Duplication

Solutions

1. If your method is simple, you don't need to test it.
2. If your method is complicated, you need to convert it into a Method Object.
3. Do not make your methods public for testing.
4. Do not use metaprogramming to avoid protection.
5. Do not move the private computation to helpers.
6. Do not use static methods for computations.

Context

We test our classes and methods.

At some point, we rely on auxiliary computations and we need to test them in a white-box way.

Sample Code

Wrong

<?

final class Star {
  
  private $distanceInParsecs;
  
  public function timeToReachLightToUs() {
    return $this->convertDistanceInParsecsToLightYears($this->distanceInParsecs);
  }
  
  private function convertDistanceInParsecsToLightYears($distanceInParsecs) {
      return 3.26 * $distanceInParsecs;
      // function is using an argument which is already available.
      // since it has private access to $distanceInParsecs
      // this is another smell indicator.

      // We cannot test this function since it is private
  }
}

Right

<?

final class Star {
  
  private $distanceInParsecs;   
  
  public function timeToReachLightToUs() {
    return new ParsecsToLightYearsConverter($this->distanceInParsecs);
  }
}

final class ParsecsToLightYearsConverter {
  public function convert($distanceInParsecs) {
      return 3.26 * $distanceInParsecs;
  }
}

final class ParsecsToLightYearsConverterTest extends TestCase {
  public function testConvert0ParsecsReturns0LightYears() {
    $this->assertEquals(0, (new ParsecsToLightYearsConverter)->convert(0));
  }
    // we can add lots of tests and rely on this object
    // So we don't need to test Star conversions.
    // We can yet test Star public timeToReachLightToUs()
    // This is a simplified scenario

}

Detection

• [x]Semi-Automatic

This is a semantic smell.

We can only find metaprogramming abuse on some unit frameworks.

• Test Smells

Conclusion

With this guide, we should always choose the method object solution.

Relations

Code Smell 21 - Anonymous Functions Abusers

Code Smell 22 - Helpers

Code Smell 18 - Static Functions

More Info

Credits

Photo by Dan Nelson on Unsplash

---

Just as it is a good practice to make all fields private unless they need greater visibility, it is a good practice to make all fields final unless they need to be mutable.

- Brian Goetz

Software Engineering Great Quotes

---

Code Smell 113 - Data Naming

Use entity domain names to model entity domain objects.

TL;DR: Don't name your variables as Data.

Problems

• Readability
• Bad Naming

Solutions

1. Use role suggesting names.
2. Find names in the Bijection.

Context

We use 'data' a lot in our variables.

We are used to doing it.

Using this kind of name favors the anemic treatment of objects.

We should think about domain-specific and role-suggesting names.

Sample Code

Wrong

if (!dataExists()) {
  return '<div>Loading Data...</div>';
}

Right

if (!peopleFound()) {
  return '<div>Loading People...</div>';
}

Detection

• [x]Semi-Automatic

We can check for this substring on our code and warn our developers.

• Readability
• Naming

Conclusion

Data is everywhere if you see the world as only data.

We can never see the data we manipulate.

We can only infer it through behavior.

We don't know the current temperature. We observe our thermometer pointing at 35 Degrees.

Our variables should reflect the domain and role they are fulfilling.

Naming them as 'data' is lazy and hinders readability.

Relations

Code Smell 01 - Anemic Models

Code Smell 65 - Variables Named after Types

More Info

What exactly is a name - Part II Rehab

---

Twenty percent of all input forms filled out by people contain bad data.

- Dennis Ritchie

Software Engineering Great Quotes

---

Code Smell 114 - Empty Class

Have you encountered classes without behavior? Classes are their behavior.

TL;DR: Remove all empty classes.

Problems

• Bijection Fault
• Namespace Polluting
• Classes used as DTOs
• Classes used as global references

Solutions

1. Remove the classes and replace them with objects instead.
2. If your classes are Anemic Exceptions, remove them.

Context

Many developers still think classes are data repositories.

They couple different behavior concepts with returning different data.

Sample Code

Wrong

class ShopItem { 
  code() { }
  description() { }                 
}

class BookItem extends ShopItem { 
   code() { return 'book' }
   description() { return 'some book'}     
}

// concrete Class has no real behavior, just return different 'data'

Right

class ShopItem { 
  constructor(code, description) {
    // validate code and description
    this._code = code;
    this._description = description;
  }
  code() { return this._code }
  description() { return this._description }                 
  // dd more functions to avoid anemic classes
  // getters are also code smells, so we need to iterate it
}

bookItem = new ShopItem('book', 'some book);
// create more items

Detection

• [x]Automatic

Several linters warn us of empty classes.

We can also make our own scripts using metaprogramming.

• Behavior

Conclusion

Classes are what they do, their behavior.

Empty classes do nothing.

Relations

Code Smell 26 - Exceptions Polluting

Code Smell 40 - DTOs

Code Smell 60 - Global Classes

Code Smell 01 - Anemic Models

More Info

Credits

Photo by Kelly Sikkema on Unsplash

---

An error arises from treating object variables (instance variables) as if they were data attributes and then creating your hierarchy based on shared attributes. Always create hierarchies based on shared behaviors, side.

- David West

Software Engineering Great Quotes

---

Code Smell 115 - Return True

Booleans are natural code smells. Returning and casting them is sometimes a mistake.

TL;DR: Don't return true or false. Be declarative.

Problems

• Readability
• Primitive Obsession
• If/Else abuse

Solutions

1. Return truth value in a declarative way
2. Replace IF With polymorphism.

Context

Dealing with low-level abstractions, we usually return booleans.

When we create complex and mature software, we start to forget about this primitive obsession and care about real-world rules and identities.

Sample Code

Wrong

boolean isEven(int num) {
     if(num%2 == 0) {
       return true;
    } else {
       return false;}        
}

Right

boolean isEven(int numberToCheck) {
  // We decouple the what (to check for even or odd)
  // With how (the algorithm)
  return (numberToCheck % 2 == 0);     
}

Detection

• [x]Automatic

Many linters can check syntactic trees and look for explicit true/value returns.

• Primitive

Conclusion

Search on code libraries for return true statements and try to replace them when possible.

Relations

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

Code Smell 118 - Return False

More Info

• How to get rid of IFs forever

Credits

Photo by engin akyurt on Unsplash

---

The good news is: Anything is possible on your computer. The bad news is: Nothing is easy.

- Ted Nelson

Software Engineering Great Quotes

---

And that’s all for now…

The next article will explain 5 more code smells!