ErikDietrich

In what has become a series of posts, I have been explaining some CodeIt.Right rules in depth.? As with the last post in the series, I’ll start off by citing two rules that I, personally, follow when it comes to static code analysis.

• 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.

Use Constants Where Appropriate

First up, let’s consider the admonition to “use constants where appropriate.”? Consider this code that I lifted from a Github project I worked on once.

I received this warning on the first two lines of code for this class.? Specifically, CodeIt.Right objects to my usage of static readonly string. If I let CodeIt.Right fix the issue for me, I wind up with the following code.

Now, CodeIt.Right seems happy.? So, what gives?? Why does this matter?

I’ll offer you the release notes of the version where CodeIt.Right introduced this rule.? If you look at the parenthetical next to the rule, you will see “performance.”? This preference has something to do with code performance.? So, let’s get specific.

When you declare a variable using const or static readonly, think in terms of magic values and their elimination.? For instance, imagine my UserAgentKey value.? Why do you think I declare that the way I did?? I did it to name that string, rather than using it inline as a “magic” string.

As a maintenance programmer, how frustrating do you find stumbling across lines of code like, “if(x == 299)”?? “What is 299, and why do we care?!”

So you introduce a variable (or, preferably, a constant) to document your intent.? In the made-up hypothetical, you might then have “if(x == MaximumCountBeforeRetry)”.? Now you can easily understand what the value means.

Either way of declaring this (constant or static, readonly field) serves the replacement purpose.? In both cases, I replace a magic value with a more readable, named one.? But in the case of static readonly, I replace it with a variable, and in the case of const, I replace it with, well, a const.

From a performance perspective, this matters.? You can think of a declaration of const as simply hard-coding a value, but without the magic.? So, when I switch to const, in my declaration, the compiler replaces every version of UserAgentKey with the string literal “user-agent”.? After compilation, you can’t tell whether I used a const or just hard-coded it everywhere.

But with a static readonly declaration, it remains a variable, even when you use it like a constant.? It thus incurs the relative overhead penalty of performing a variable lookup at runtime.? For this reason, CodeIt.Right steers you toward considering making this a constant.

Parameter Names Should Match Base Declaration

For the next rule, let’s return to the Github scraper project from the last example.? I’ll show you two snippets of code.? The first comes from an interface definition and the second from a class implementing that interface.? Pay specific attention to the method, GetRepoSearchResults.

If you take a look at the parameter names, it probably won’t surprise you to see that they do not match.? Therein lies the problem that CodeIt.Right has with my code.? It wants the implementing class to match the interface definition (i.e. the “base”).? But why?

In this case, we have a fairly simple answer.? Having different names for the conceptually same method creates confusion.

Specifically, maintainers will struggle to understand whether you meant to override or overload the method.? In our mind’s eyes, identical method signatures signals polymorphic approaches, while same name, different parameters signals overload.? In a sense, changing the name of a variable fakes maintenance programmers out.

Do Not Declare Externally Visible Instance Fields

I don’t believe we need a screenshot for this one.? Consider the following trivial code snippet.

public class SomeClass
{
    public string _someVariable;
}

This warning says, “don’t do that.”? More specifically, don’t declare an instance field with external (to the type) visibility.? The question is, “why not?”

If you check out the Microsoft guidance on the subject, they explain that, the “use of a field should be as an implementation detail.”? In other words, they contend that you violate encapsulation by exposing fields.? Instead, they say, you should expose this via a property (which simply offers syntactic sugar over a method).

Instead of continuing with abstract concepts, I’ll offer a concrete example.? Imagine that you want to model a family and you declare an integer field called _numberOfChildren. That works fine initially, but eventually you encounter the conceptually weird edge case where someone tries to define a family with -1 children.? With an integer field, you can technically do this, but you want to prevent that from happening.

With clients of your class directly accessing and setting this field, you wind up having to go install this guard logic literally everywhere your clients interact with the field.? But had you hidden the field behind a property, you could simply add logic to the property setter wherein you throw an exception on an attempt to set a negative value.

This rule attempts to help you future-proof your code and follow good OO practice.

Until Next Time

Somewhat by coincidence, this post focused heavily on the C# flavor of object-oriented programming.? We looked at constants versus field access, but then focused on polymorphism and encapsulation.

I mention this because I find it interesting to see where static analyzers take you.? Follow along for the rest of the series and, hopefully, you’ll learn various useful nuggets about the language you use.

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

A little while back, I started a post series explaining some of the CodeIt.Right rules.? I led into the post with a narrative, which I won’t retell.? But I will reiterate the two rules that I follow when it comes to 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.

Because I follow these two rules, I find myself researching every fix suggested to me by my tooling.? And, since I’ve gone to the trouble of doing so, I’ll save you that same trouble by explaining some of those rules today.? Specifically, I’ll examine 3 more CodeIt.Right rules today and explain the rationale behind them.

Mark assemblies CLSCompliant

If you develop in .NET, you’ve no doubt run across this particular warning at some point in your career.? Before we get into the details, let’s stop and define the acronyms.? “CLS” stands for “Common Language Specification,” so the warning informs you that you need to mark your assemblies “Common Language Specification Compliant” (or non-compliant, if applicable).

Okay, but what does that mean?? Well, you can easily forget that many programming languages target the .NET runtime besides your language of choice.? CLS compliance indicates that any language targeting the runtime can use your assembly.? You can write language specific code, incompatible with other framework languages.? CLS compliance means you haven’t.

Want an example?? Let’s say that you write C# code and that you decide to get cute.? You have a class with a “DoStuff” method, and you want to add a slight variation on it.? Because the new method adds improved functionality, you decide to call it “DOSTUFF” in all caps to indicate its awesomeness.? No problem, says the C# compiler.

And yet, if you you try to do the same thing in Visual Basic, a case insensitive language, you will encounter a compiler error.? You have written C# code that VB code cannot use.? Thus you have written non-CLS compliant code.? The CodeIt.Right rule exists to inform you that you have not specified your assembly’s compliance or non-compliance.

To fix, go specify.? Ideally, go into the project’s AssemblyInfo.cs file and add the following to call it a day.

[assembly:CLSCompliant(true)]

But you can also specify non-compliance for the assembly to avoid a warning.? Of course, you can do better by marking the assembly compliant on the whole and then hunting down and flagging non-compliant methods with the attribute.

Specify IFormatProvider

Next up, consider a warning to “specify IFormatProvider.”? When you encounter this for the first time, it might leave you scratching your head.? After all, “IFormatProvider” seems a bit… technician-like.? A more newbie-friendly name for this warning might have been, “you have a localization problem.”

For example, consider a situation in which some external supplies a date.? Except, they supply the date as a string and you have the task of converting it to a proper DateTime so that you can perform operations on it.? No problem, right?

var properDate = DateTime.Parse(inputString);

That should work, provided provincial concerns do not intervene.? For those of you in the US, “03/02/1995” corresponds to March 2nd, 1995.? Of course, should you live in Iraq, that date string would correspond to February 3rd, 1995.? Oops.

Consider a nightmare scenario wherein you write some code with this parsing mechanism.? Based in the US and with most of your customers in the US, this works for years.? Eventually, though, your sales group starts making inroads elsewhere.? Years after the fact, you wind up with a strange bug in code you haven’t touched for years.? Yikes.

By specifying a format provider, you can avoid this scenario.

Nested types should not be visible

Unlike the previous rule, this one’s name suffices for description.? If you declare a type within another type (say a class within a class), you should not make the nested type visible outside of the outer type.? So, the following code triggers the warning.

public class Outer
{
    public class Nested
    {

    }
}

To understand the issue here, consider the object oriented principle of encapsulation.? In short, hiding implementation details from outsiders gives you more freedom to vary those details later, at your discretion.? This thinking drives the rote instinct for OOP programmers to declare private fields and expose them via public accessors/mutators/properties.

To some degree, the same reasoning applies here.? If you declare a class or struct inside of another one, then presumably only the containing type needs the nested one.? In that case, why make it public?? On the other hand, if another type does, in fact, need the nested one, why scope it within a parent type and not just the same namespace?

You may have some reason for doing this — something specific to your code and your implementation.? But understand that this is weird, and will tend to create awkward, hard-to-discover code.? For this reason, your static analysis tool flags your code.

Until Next Time

As I said last time, you can extract a ton of value from understanding code analysis rules.? This goes beyond just understanding your tooling and accepted best practice.? Specifically, it gets you in the habit of researching and understanding your code and applications at a deep, philosophical level.

In this post alone, we’ve discussed language interoperability, geographic maintenance concerns, and object oriented design.? You can, all too easily, dismiss analysis rules as perfectionism.? They aren’t; they have very real, very important applications.

Stay tuned for more posts in this series, aimed at helping you understand your tooling.

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

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.