CodeIt.Right Rules Explained, Part 4

Editorial Note: I originally wrote this post for the SubMain blog.  You can check out the original here, at their site.  While you’re there, have a look at CodeIt.Right to help you perform automated code reviews.  

Today, I’ll do another installment of the CodeIt.Right Rules, Explained series.  I have now made four such posts in this series.  And, as always, I’ll start off by citing my two personal rules about static analysis guidance.

• Never implement a suggested fix without knowing what makes it a fix.
• Never ignore a suggested fix without understanding what makes it a fix.

It may seem as though I’m playing rhetorical games here.  After all, I could simply say, “learn the reasoning behind all suggested fixes.”  But I want to underscore the decision you face when confronted with static analysis feedback.  In all cases, you must actively choose to ignore the feedback or address it.  And for both options, you need to understand the logic behind the suggestion.

In that spirit, I’m going to offer up explanations for three more CodeIt.Right rules today.

Type that contains only static members should be sealed.

Let’s start here with a quick example.  I think this picture will suffice for some number of words, if not necessarily one thousand.

Here, I’ve laid a tiny seed for a Swiss Army Knife, “utils” class.  Presumably, I will continue to dump any method I think might help me with Linq into this class.  But for now, it contains only a single method to make things easy to understand.  (As an aside, I discourage “utils” classes as a practice.  I’m using this example because everyone reading has most assuredly seen one of these things at some point.)

When you run CodeIt.Right analysis on this code, you will find yourself confronted with a design issue.  Specifically, “types that contain only static members should be sealed.”

You probably won’t have a hard time discerning how to remedy the situation.  Adding the “sealed” modifier to the class will do the trick.  But why does CodeIt.Right object?

The Microsoft guidelines contain a bit more information.  They briefly explain that static analyzers make an inference about your design intent, and that you can better communicate that intent by using the “sealed” keyword.  But let’s unpack that a bit.

When you write a class that has nothing but static members, such as a static utils class, you create something with no instantiation logic and no state.  In other words, you could instantiate “a LinqUtils,” but you couldn’t do anything with it.  Presumably, you do not intend that people use the class in that way.

But what about other ways of interacting with the class, such as via inheritance?  Again, you could create a LinqUtilsChild that inherited from LinqUtils, but to what end?  Polymorphism requires instance members, and non exist here.  The inheriting class would inherit absolutely nothing from its parent, making the inheritance awkward at best.

Thus the intent of the rule.  You can think of it telling you the following.  “You’re obviously not planning to let people use inheritance with you, so don’t even leave that door open for them to possibly make a mistake.”

So when you find yourself confronted with this warning, you have a simple bit of consideration.  Do you intend to have instance behavior?  If so, add that behavior and the warning goes away.  If not, simply mark the class sealed.

Async methods should have async suffix.

Next up, let’s consider a rule in the naming category.  Specifically, when you name an async method with suffixing “async” on its name, you see the warning.  Microsoft declares this succinctly in their guidelines.

By convention, you append “Async” to the names of methods that have an async modifier.

So, CodeIt.Right simply tells us that we’ve run afoul of this convention.  But, again, let’s dive into the reasoning behind this rule.

When Microsoft introduced this programming paradigm, they did so in a non-breaking release.  This caused something of a conundrum for them because of a perfectly understandable language rule stating that method overloads cannot vary only by a return type.  To take advantage of the new language feature, users would need to offer the new, async methods, and also backward compatibility with existing method calls.  This put them in the position of needing to give the new, asnyc methods different names.  And so Microsoft offered guidance on a convention for doing so.

I’d like to make a call-out here with regard to my two rules at the top of each post.  This convention came about because of expediency and now sticks around for convention’s sake.  But it may bother you that you’re asked to bake a keyword into the name of a method.  This might trouble you in the same way that a method called “GetCustomerNumberString()” might bother you.  In other words, while I don’t advise you go against convention, I will say that not all warnings are creatd equally.

Always define a global error handler.

With this particular advice, we dive into warnings specific to ASP.  When you see this warning, it concerns the Global.asax file.  To understand a bit more about that, you can read this Stack Overflow question.  In short, Global.asax allows you to define responses to “system level” in a single place.

CodeIt.Right is telling you to define just such an event — specifically one in response to the “Application_Error” event.  This event occurs whenever an exception bubbles all the way up without being trapped anywhere by your code somewhere.  And, that’s a perfectly reasonable state of affairs — your code won’t trap every possible exception.

CodeIt.Right wants you to define a default behavior on application errors.  This could mean something as simple as redirecting to a page that says, “oops, sorry about that.”  Or, it could entail all sorts of robust, diagnostic information.  The important thing is that you define it and that it be consistent.  You certainly don’t want to learn from your users what your own application does in response to an error.

So spent a bit of time defining your global error handling behavior.  By all means, trap and handle exceptions as close to the source as you can.  But always make sure to have a backup plan.

Until Next Time

In this post, I ran the gamut across concerns.  I touched on an object-oriented design concern.  Then, I went into a naming consideration involving async, and, finally, I talked specifically about ASP programming considerations.

I don’t have a particular algorithm for the order in which I cover these subjects.  But, I like the way this shook out.  It goes to show you that CodeIt.Right covers a lot of ground, across a lot of different landscapes of the .NET world.