DaedTech

Stories about Software

By

CodeIt.Right Rules, Explained Part 3

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, take a look at CodeIt.Right and see how you can automate checks for and enforcement of these rules.

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

1 Comment
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Mathew Morrey-Clark
Mathew Morrey-Clark
6 years ago

I’ve always worked on the principle that you should never use non-private constants unless they are truly universally constant – e.g. mathematical or scientific constants. Anything that is liable to change (e.g. business rules such as number of retries, cache size, category names, etc.) should always be static readonly. Unless you are in a super-performance critical application, you’re not going to notice the advantage, but there is a gotcha with constants… Because they are compiled inline, if I define a constant in library1 then use it in library2, changing that constant will mean re-compiling and re-deploying both libraries. With a… Read more »