CodeIt.Right

In what has become roughly a monthly affair, I’ll do another CodeIt.Right Rules Explained post today.  This marks the ninth such post and roughly nine months of the series.  In a now-familiar refrain, I’ll start off by citing my two personal rules about static analysis followed by a brief explanation.

• 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 might seem I’m playing rhetorical games here.  After all, I could condense this to say “learn the reasoning behind all suggested fixes.”  But I say it the way that I do to call your attention to a decision that you face when it comes to static analysis warnings.  Every time you encounter a warning, you must either choose to ignore the feedback or to address it.  And regardless of which you chose, you should really understand the logic behind the suggestion.  Otherwise, how can you possibly make the right call?

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

Avoid Unsealed Attributes

Let’s start off with something conceptually easy to understand, at least in terms of the problem.  You’ve no doubt encountered the attribute construct before, whether you realize it or not.  Here’s a quick code example.

[TestMethod]
public void Adding_10_And_15_Returns_25()
{
    var calculator = new Calculator();

    int result = calculator.Add(10, 15);

    Assert.AreEqual<int>(25, result);
}

That thing at the very top?  TestMethod?  That’s called an attribute.

In .NET, attributes amount to declarative metadata about the code element they describe.  For instance, we use the attribute in the code sample to declare that this is a test method.  You write this code to opt into the MS Test framework.  The framework then looks for that attribute and treats any method decorated with it as a unit test method.

How do you get access to this magic?  Let’s look at a trivial, ridiculous, and hopefully memorable example.  First, I declare the attribute.

public class ApprovedByErikAttribute : Attribute
{

}

With that on the books, I can now meander through the codebase, making my opinion felt.  Let’s say I encounter a class called Circle.  It’s a bit simplistic, but it’s off to a good start.  I approve this class!  Let’s let everyone know.

[ApprovedByErik]
public class Circle
{
    private readonly int _radius;

    public Circle(int radius)
    {
        _radius = radius;
    }
}

Alright, everything looks great here.  Except I run a CodeIt.Right analysis on this and I see a warning: “avoid unsealed attributes.”  Well, inheritance is certainly less fashionable these days, but CodeIt.Right flags this as a performance issue.  What gives?

Well, you access this attribute metadata via reflection, which, as it turns out, is expensive in and of itself.  You have to pay the piper for this magic.  But doing this with unsealed attributes is even more expensive since the .NET framework methods for accessing attribute information search the entire inheritance hierarchy by default.  Sealing your class basically tells them not to bother looking any further.  It’s also a really easy fix: just add the keyword “sealed” to your attribute class and call it a day.

Avoid Excessive Complexity

Let’s change gears here and switch over to a CodeIt.Right rule under the heading “maintainability.”  This one counsels you to “avoid excessive complexity,” and I won’t include a code sample here.  Why?  Because in order to do that, I’d have to come up with a ridiculous method that would take up half of the space of this post.  I’ll come back to that, though.

When CodeIt.Right refers here to “complexity,” it’s not offering some vague, subjective take.  Rather, it refers to something measurable and specific.  I’m talking about the idea of cyclomatic complexity.  Cyclomatic complexity measures the number of independent paths through a method.  So if you have a method that just returns the sum of two integers, then you have cyclomatic complexity of one.  But if you add a guarding if condition, you now have a cyclomatic complexity of two.

To conceptualize the idea, think in terms of control flow logic.  If and else conditionals add to a method’s cyclomatic complexity.  So do while loops, for loops, ternary operators, and switch statements.  The more decision logic occurring in a method, triggering different ways to step through it, the higher the cyclomatic complexity.

Out of the box, CodeIt.Right will flag methods with cyclomatic complexity of more than 25.  That means it will flag any methods with more than 25 independent paths through them.  Make no mistake — that is an insanely complex method that will cause you maintenance headaches.  And now you can understand why I wouldn’t want to dream up and post such a thing.  It’d just be a tortured rat’s nest of nested loops, conditionals, and perhaps even some goto logic for good measure.

Enforce Warning Level 4

Last up today, let’s look at something a bit off the beaten path.  This falls under the heading of “general” in CodeIt.Right’s rule categorizations.  And it speaks to properties of your project, rather than of your code.

If you right click a project in Visual Studio and select “project properties” (or use keyboard shortcut alt + enter), you get a screen that lets you modify aspects of the project itself.  Select the “build” tab, and you can modify settings about how MS Build compiles the project.  Of interest today, we have the “Warning Level” setting under “Errors and warnings,” as shown here.

By default, your projects come with warning level 4 pre-selected.  And 4 is also the highest level of warnings.  You can check out the details of each warning level here.

The warning messages to which it refers show up in your “errors” window when you build your project.  That window will show you any compiler errors, but also compiler warnings, such as unreachable code or unused variables.

When you select something less than 4, you’re telling your build to show you fewer warning messages.  If you go all the way down to 0, you’re telling it show you no warnings at all: “If you’re technically able to build this, then build it and tell me nothing.”  CodeIt.Right warns you about this practice because it’s generally a bad idea to ignore compiler warnings.  The vast majority of the time, they’re warning you about something legitimately wrong with your code.

If, on occasion, the compiler warning system yields some false negatives, you have other recourse to ignore them.  You don’t need to engage in blanket disabling.  CodeIt.Right warns you about this course of action because it’s almost certainly overkill for what you want to accomplish.

Until Next Time

I’ve tried to offer yet another eclectic mix of code issues in this post.  I started with a lesser known bit about defining your own attributes that actually relates to performance.  Then I took you through the general issue of high cyclomatic complexity and wound up with a general suggestion not to ignore wholesale warnings in your codebase.

And that last bit of advice, in fact, parallels my general advice on static analysis.  The compiler is yet another form of static analyzer, and you should probably heed its advice or else have a very, very good reason not to.

Learn more how CodeIt.Right can help you automate code reviews and improve the quality of your code.