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