paint-brush
How to Find the Stinky Parts of Your Code [Part XXXVII]by@mcsee
112 reads

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

by Maximiliano ContieriMay 15th, 2023
Read on Terminal Reader
Read this story w/o Javascript
tldt arrow

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.)

People Mentioned

Mention Thumbnail
featured image - How to Find the Stinky Parts of Your Code [Part XXXVII]
Maximiliano Contieri HackerNoon profile picture

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

Solutions

  1. Make the class public
  2. Keep the public class under your own namespace/module.
  3. 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.

Tags

  • Hierarchies

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

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

Solutions

  1. 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.

Tags

  • Duplication

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

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

  1. 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

Tags

  • Comments

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

  1. 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

Tags

  • Exceptions

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

  1. Cover the cases with tests to see if they halt
  2. Use algorithms instead of regular expressions
  3. 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)
        }
    }
}

Detection

  • [x]Semi-Automatic

Many languages avoid this kind of regular expression.

We can also scan the code for this vulnerability.

Tags

  • Security

Conclusion

Regular Expressions are tricky and hard to debug.

We should avoid them as much as possible.

Relations

Code Smell 41 - Regular Expression Abusers

More Info

Catastrophic backtracking: how can a regular expression cause a ReDoS vulnerability?

https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS

Runaway Regular Expressions: Catastrophic Backtracking

Disclaimer

Code Smells are just my opinion.

Credits

Photo by engin akyurt on Unsplash


Some people, when confronted with a problem, think “I know, I’ll use regular expressions.” Now they have two problems.

Jamie Zawinski


5 more code smells are coming soon…