I've heard tell of a social experiment conducted with monkeys. It may or may
not be apocryphal, but it illustrates an interesting point. So, here goes.
Primates and Conformity
A group of monkeys inhabited a large enclosure, which included a platform in the middle,
accessible by a ladder. For the experiment, their keepers set a banana on the
platform, but with a catch. Anytime a monkey would climb to the platform, the
action would trigger a mechanism that sprayed the entire cage with freezing cold water.
The smarter monkeys quickly figured out the correlation and actively sought to prevent
their cohorts from triggering the spray. Anytime a monkey attempted to climb
the ladder, they would stop it and beat it up a bit by way of teaching a lesson.
But the experiment wasn't finished.
Once the behavior had been established, they began swapping out monkeys. When
a newcomer arrived on the scene, he would go for the banana, not knowing the social
rules of the cage. The monkeys would quickly teach him, though. This continued
until they had rotated out all original monkeys. The monkeys in the cage would
beat up the newcomers even though they had never experienced the actual negative
consequences.
Now before you think to yourself, "stupid monkeys," ask yourself how much better you'd
fare. This
video shows that humans have the same instincts as our primate cousins.
Static Analysis and Conformity
You might find yourself wondering why I told you this story. What does it have
to do with software tooling and static analysis?
Well, I find that teams tend to exhibit two common anti-patterns when it comes to
static analysis. Most prominently, they tune out warnings without due diligence.
After that, I most frequently see them blindly implement the suggestions.
I tend to follow two rules when it comes to my interaction with static analysis tooling.
-
Never implement a suggested fix without knowing what makes it a fix.
-
Never ignore a suggested fix without understanding what makes it a fix.
You syllogism buffs out there have, no doubt, condensed this to a single rule.
Anytime you encounter a suggested fix you don't understand, go learn about it.
Once you understand it, you can implement the fix or ignore the suggestion with eyes
wide open. In software design/architecture, we deal with few clear cut rules
and endless trade-offs. But you can't speak intelligently about the trade-offs
without knowing the theory behind them.
Toward that end, I'd like to facilitate that warning for some CodeIt.Right rules
today. Hopefully this helps you leverage your tooling to its full benefit.
Abstract types should not have public constructors
First up, consider the idea of abstract types with public constructors.
public abstract class Shape
{ protected ConsoleColor
_color; public Shape(ConsoleColor
color) { _color = color;
} } public class Square
: Shape { public int SideLength
{ get; set;
} public Square(ConsoleColor
color) : base(color)
{ } }
CodeIt.Right will ding you for making the Shape constructor public (or
internal -- it wants protected). But why?
Well, you'll quickly discover that CodeIt.Right has good company in the form of the
.NET Framework guidelines and FxCop rules. But that just shifts the discussion
without solving the problem. Why does everyone seem not to like this
code?
First, understand that you cannot instantiate Shape, by design. The "abstract"
designation effectively communicates Shape's incompleteness. It's more of a template than
a finished class in that creating a Shape makes no sense without the added specificity
of a derived type, like Square.
So the only way classes outside of the inheritance hierarchy can interact with Shape
indirectly, via Square. They create Squares, and those Squares decide how to
go about interacting with Shape. Don't believe me? Try getting around
this. Try creating a Shape in code or try deleting Square's constructor and
calling new Square(color). Neither will compile.
Thus, when you make Shape's constructor public or internal, you invite users of your
inheritance hierarchy to do something impossible. You engage in false
advertising and you confuse them. CodeIt.Right is helping you avoid this
mistake.
Do not catch generic exception types
Next up, let's consider the wisdom, "do not catch generic exception types."
To see what that looks like, consider the following code.
public bool MergeUsers(int user1Id, int user2Id)
{ try { var user1 = _userRepo.Get(user1Id); var user2 = _userRepo.Get(user2Id);
user1.MergeWith(user2); _userRepo.Save(user1); _userRepo.Delete(user2); return true;
} catch(Exception
ex) { _logger.Log($"Exception
{ex.Message} occurred."); return false;
} }
Here we have a method that merges two users together, given their IDs. It accomplishes
this by fetching them from some persistence ignorance scheme, invoking a merge operation,
saving the merged one and deleting the vestigial one. Oh, and it wraps the whole
thing in a try block, and then logs and returns false should anything fail.
And, by anything, I mean absolutely anything. Business rules make merge
impossible? Log and return false. Server out of memory? Log it and
return false. Server hit by lightning and user data inaccessible? Log
it and return false.
With this approach, you encounter two categories of problem. First, you fail
to reason about or distinguish among the different things that might go wrong.
And, secondly, you risk overstepping what you're equipped to handle here. Do
you really want to handle fatal system exceptions right smack in the heart
of the MergeUsers business logic?
You may encounter circumstances where you want to handle everything, but probably
not as frequently as you think. Instead of defaulting to this catch all, go
through the exercise of reasoning about what could go wrong here and what you want
to handle.
Avoid language specific type names in parameters
If you see this violation, you probably have code that resembles the following.
(Though, hopefully, you wouldn't write this actual method)
public int Add(int xInt, int yInt)
{ return xInt + yInt;
}
CodeIt.Right does not like the name "int" in the parameters and this reflects a .NET
Framework guideline.
Here, we find something a single language developer may not stop to consider.
Specifically, not all languages that target the .NET framework use the same type name
conveniences. You say "int" and a VB developer says "Integer." So if a
VB developer invokes your method from a library, she may find this confusing.
That said, I would like to take this one step further and advise that you avoid baking
types into your parameter/variable names in general. Want to know why?
Let's consider a likely outcome of some project manager coming along and saying, "we
want to expand the add method to be able to handle really big numbers." Oh,
well, simple enough!
public long Add(long xInt, long yInt)
{ return xInt + yInt;
}
You just needed to change the datatypes to long, and voilà! Everything went
perfectly until someone asked you at code review why you have a long called "xInt."
Oops. You totally didn't even think about the variable names.
You'll be more careful next time. Well, I'd advise avoiding "next time" completely
by getting out of this naming habit. The IDE can tell you the type of a variable
-- don't encode it into the name redundantly.
Until Next Time
As I said in the introductory part of the post, I believe huge value exists in understanding
code analysis rules. You make better decisions, have better conversations, and
get more mileage out of the tooling. In general, this understanding makes you
a better developer. So I plan to continue with these explanatory posts from
time to time. Stay tuned!
Learn
more how CodeIt.Right can help you automate code reviews and improve your code quality.
About the Author
Erik Dietrich
I'm a passionate software developer and active blogger. Read about me at my
site. View
all posts by Erik Dietrich
