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 QuoteseSaver {
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
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
This article is part of the CodeSmell Series.
Top comments (3)
In this case I'd skip the else keyword completely.
Nothing is executed after throw.
It is similar to an early return.
If the style allows it, I'd skip the curly braces, but only if the if and throw fit into one line
I assume
should go after the closing } on the if/elseif chain?
exactly.
I spotted it.
Thank you!