108 reads

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

by Maximiliano ContieriMay 15th, 2023

Too Long; Didn't Read

Most of these smells are just hints of something that might be wrong. Therefore, they are not required to be fixed per se… (You should look into it, though.)

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

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. Therefore, they are not required to be fixed per se… (You should look into it, though.)

Previous Code Smells

You can find all the previous code smells (Part i - XXXVI) here.

Let's continue...

Code Smell 181 - Nested Classes

Nested or Pseudo-Private Classes seem great for hiding implementation details.

TL;DR: Don't use nested classes

Problems

Bijection fault to real-world concepts.
Lack of testability
Lack of reuse
Scopes and namespaces complexity

Solutions

Make the class public
Keep the public class under your own namespace/module.
Use a Facade to the external world to hide it.

Context

Some languages allow us to create private concepts that only live inside a more significant idea.

These classes are harder to test, harder to debug, and reuse.

Sample Code

Wrong


class Address {

  String description = "Address: ";

  public class City {
    String name = "Doha";
  }
}


public class Main {

  public static void main(String[] args) {

    Address homeAddress = new Address();
    Address.City homeCity = homeAddress.new City();
    System.out.println(homeAddress.description + homeCity.name);
  }
}

// The output is "Address: Doha"
//
// If we change privacy to 'private class City' 
//
// We get an error " Address.City has private access in Address"

Right


class Address {
  String description = "Address: ";
}

 
class City {
  String name = "Doha";
}


public class Main {
  public static void main(String[] args) {
    Address homeAddress = new Address();
    City homeCity = new City();
    System.out.println(homeAddress.description + homeCity.name);
  }
}

// The output is "Address: Doha"
//
// Now we can reuse and test the City concept

Detection

[x]Automatic

Since this is a language feature, we can detect it and avoid its usage.

Conclusion

Many features are bloated with complex features.

We seldom need these new pop culture features.

We need to keep a minimal set of concepts.

More Info

W3 schools

Disclaimer

Code Smells are just my opinion.

Credits

Photo by Dana Ward on Unsplash

Developers are drawn to complexity like moths to a flame, frequently with the same result.

Neal Ford

Software Engineering Great Quotes

Code Smell 182 - Over Generalization

We are refactoring fans. Sometimes we need to stop and think.

TL;DR: Don't make generalizations beyond real knowledge.

Problems

Overgeneralization
Bijection violation

Solutions

Think before making structural generalizations

Context

Refactoring is not just looking at structural code.

We need to refactor behavior and check if it needs an abstraction.

Sample Code

Wrong

fn validate_size(value: i32) {
    validate_integer(value);
}

fn validate_years(value: i32) {
    validate_integer(value);
}

fn validate_integer(value: i32) {
    validate_type(value, :integer);
    validate_min_integer(value, 0);
}

Right

fn validate_size(value: i32) {
 	validate_type(value, Type::Integer);
	validate_min_integer(value, 0);
}
	
fn validate_years(value: i32) {
	validate_type(value, Type::Integer);
	validate_min_integer(value, 0);
}
	
// Duplication is accidental, therefore we should not abstract it

Detection

[x]Manual

This is a semantic smell.

Conclusion

Software development is a thinking activity.

We have automated tools to help and assist us. We need to be in charge.

Relations

Code Smell 46 - Repeated Code

More Info

DRY—The Evils of Duplication

Disclaimer

Code Smells are just my opinion.

Credits

Photo by Matthew Henry on Unsplash

Duplication is far cheaper than the wrong abstraction.

Sandi Metz

Code Smell 183 - Obsolete Comments

Comments are a code smell. Obsolete comments are a real danger and nobody maintains what can't be executed.

TL;DR: Don't trust comments. They are dead.

Problems

Bad documentation
Maintainability

Solutions

Replace comments with tests

Refactorings

Refactoring 005 - Replace Comment with Function Name

Context

We know comments add almost no value to our code.

We need to restrict comments only to very important decisions.

Since most people change logic and forget to update comments they might become obsolete easily.

Sample Code

Wrong

void Widget::displayPlugin(Unit* unit)
{

 	// TODO the Plugin will be modified soon, 
	// so I don't implement this right now

 	if (!isVisible) {
		// hide all widgets
		return;
 	}

}

Right

void Widget::displayPlugin(Unit* unit)
{ 
	if (!isVisible) {
		return;
 	}
}

Detection

[x]Semi-Automatic

We can warn for comments in our code and try to remove them.

Exceptions

Very important design decisions

Conclusion

We need to think before adding a comment. Once It is in the codebase is beyond our control and can start to lie anytime.

Relations

Code Smell 05 - Comment Abusers

Code Smell 152 - Logical Comment

Code Smell 151 - Commented Code

Disclaimer

Code Smells are just my opinion.

Credits

Photo by Volodymyr Hryshchenko on Unsplash

Obsolete comments tend to migrate away from the code they once described. They become floating islands of irrelevance and misdirection in the code.

Bob Martin

Software Engineering Great Quotes

Code Smell 184 - Exception Arrow Code

Arrow code is a code smell. Exception polluting is another. This is a mortal combination.

TL;DR: Don't cascade your exceptions

Problems

Readability
Complexity

Solutions

Rewrite the nested clauses

Context

In the same way arrow code is hard to read, handling exceptions is a usual case when we must address the topics in a cascade way.

Sample Code

Wrong

class QuotesSaver {
    public void Save(string filename) {
        if (FileSystem.IsPathValid(filename)) {
            if (FileSystem.ParentDirectoryExists(filename)) {
                if (!FileSystem.Exists(filename)) {
                    this.SaveOnValidFilename(filename);
                } else {
                    throw new I0Exception("File exists: " + filename);
                }
            } else {
                throw new I0Exception("Parent directory missing at " + filename);
            }
        } else {
            throw new ArgumentException("Invalid path " + filename);
        }
    }
}

Right

public class QuotesSaver {
    public void Save(string filename) {
        if (!FileSystem.IsPathValid(filename)) {
            throw new ArgumentException("Invalid path " + filename);
        } else if (!FileSystem.ParentDirectoryExists(filename)) {
            throw new I0Exception("Parent directory missing at " + filename);
        } else if (FileSystem.Exists(filename)) {
             throw new I0Exception("File exists: " + filename);
        }
        this.SaveOnValidFilename(filename);
    }
}

Detection

[x]Semi-Automatic

Some linters warn us when we have this kind of complexity

Conclusion

Exceptions are less critical than normal cases.

If we need to read more exceptional code than normal then it is time to improve our code.

Relations

Code Smell 102 - Arrow Code

Code Smell 26 - Exceptions Polluting

Disclaimer

Code Smells are just my opinion.

Credits

Photo by Remy Gieling on Unsplash

An error doesn't become a mistake until you refuse to correct it.

Orlando Aloysius Battista

Software Engineering Great Quotes

Code Smell 185 - Evil Regular Expressions

Regular expressions are a code smell. Sometimes also a vulnerability

TL;DR: Try to minimize Regular Expression's recursive rules.

Problems

Security Issues
Readability
Premature Optimization

Solutions

Cover the cases with tests to see if they halt
Use algorithms instead of regular expressions
Add timeout handlers

Context

This is known as ReDos attack, a subtype of a Denial of Service attack.

ReDoS attacks can be divided into two types:

A string with an evil pattern is passed to an application. Then this string is used as a regex, which leads to ReDoS.

A string with a vector attack format is passed to an application. Then this string is evaluated by a vulnerable regex, which leads to ReDoS.

Sample Code

Wrong

package main

import (
    "regexp"
    "fmt"
)

func main() {
    var re = regexp.MustCompile(`^(([a-z])+.)+[A-Z]([a-z])+$`)
    var str = `aaaaaaaaaaaaaaaaaaaaaaaa!`
    
    for i, match := range re.FindAllString(str, -1) {
        fmt.Println(match, "found at index", i)
    }
}

Right

package main

import (
    "fmt"
    "strings"
)

func main() {
    var str = `aaaaaaaaaaaaaaaaaaaaaaaa!`
    
    words := strings.Fields(str)
    
    for i, word := range words {
        if len(word) >= 2 && word[0] >= 'a' && word[0] <= 'z' && word[len(word)-1] >= 'A' 
            && word[len(word)-1] <= 'Z' {
                fmt.Println(word, "found at index", i)
        }
    }
}