190 reads

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

by Maximiliano ContieriSeptember 26th, 2022

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 classic, but it's not required to be fixed. The smell is a hint that something is wrong, but you should look at it more closely if you want to improve your code smell. For example, the smell of Code Smells: "Stinky Smells"

Company Mentioned

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

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

Avoid altering the collections
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

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

If your method is simple, you don't need to test it.
If your method is complicated, you need to convert it into a Method Object.
Do not make your methods public for testing.
Do not use metaprogramming to avoid protection.
Do not move the private computation to helpers.
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.

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

Use role suggesting names.
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.

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

Remove the classes and replace them with objects instead.
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.

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

Return truth value in a declarative way
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);     
}