CodeIt.Right Rules Explained, Part 6

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, follow the tag to have a look at the rest of the series.

Let’s do another installment of the CodeIt.Right Rules Explained series.  Today we have post number six in that series, with three more rules.  As always, I’ll start off with my two personal rules about static analysis guidance, along with an explanation for them.

• 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 play rhetorical games here.  After all, I could just 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 suggestions.

In that spirit, I’ll offer up explanations for our three rules without further ado.

Do Not Raise Reserved Exception Types

The guard clause seems to stand the test of time in application development.  You might also know it as defensive programming.  Whatever you call it, it looks like this.

public static string SpacePascalCase(this string target)
{
    if(target == null)
        throw new ArgumentNullException(nameof(target));

    return Regex.Replace(target, @"\B[A-Z]", " $0");
}

These types of clauses exist to preserve preconditions (or invariants) for methods.  You reason to yourself that the wrong sorts of arguments could create problems, so you fail fast.

Because of the ubiquity of these types of guards, you understand clearly from the other side.  If you call a method with a null argument and receive an ArgumentNullException, you understand quickly.  Contrast this with calling a method and receiving NullReferenceException.  With the former, a quick fix happens.  With the latter, you sigh because you know something more subtle went wrong in the code.

That all holds true unless you do something like this.

public static string SpacePascalCase(this string target)
{
    if(target == null)
        throw new NullReferenceException();

    return Regex.Replace(target, @"\B[A-Z]", " $0");
}

Now your users will experience some confusion.  This happens because you’re raising what’s called a reserved exception type.  And when you do that, CodeIt.Right lets you know about it.

These reserved exceptions create two distinct categories of problem when you raise them.  First, some of them reside high up in the in inheritance hierarchy — exception itself, for example.  When you raise an exception like this, you force people using your code to deal with every imaginable class of exception out there, whether they want to or not.

Secondly, you have exceptions conventionally thrown only by the common language runtime (CLR).  The code above represents such a situation.  When you throw these sorts of exceptions, you confuse people using your code.  They’ll think you made a mistake.

Properties Should Not Return Arrays

Encapsulation represents one of the bedrock principles of object-oriented programming.  The whole concept of member visibility and its prominence in .NET languages speaks to that.  Most OOP developers adhere to the conventional wisdom of making things as minimally visible as possible.  Expose only what you must.

Because of this tendency, we get the language convenience of properties.  Using properties, we can hide class fields so that we can later change their implementation, among other things.  And using properties, we spend a good bit of time reasoning about who should have the ability to touch what.  For instance, consider the following code.

public class FileProcessor
{
    private readonly FileReader _reader;

    public FileReader Reader { get { return _reader; } }
        
    public FileProcessor(FileReader reader)
    {
        _reader = reader;
    }

You see this sort of thing a lot.  The FileProcessor class protects its FileReader.  Sure, consumers of the class can get at it to view it.  But after instantiating, they can’t go in and null it out or anything.  We use properties in this fashion to manage visibility and access.

But if we try to do that with arrays, things get decidedly weirder.

public class FileProcessor
{
    private readonly FileReader[] _readers;

    public FileReader[] Readers { get { return _readers; } }
        
    public FileProcessor(FileReader[] readers)
    {
        _readers = readers;
    }

As you can see, we employ the same basic concept.  But this time, instead of a reader, we have an array of them.  And we expose that array via a property.

Look what we can now do from client code.

public void FileProcessorClient()
{
    var readers = new FileReader[] { new FileReader(), new FileReader() };
    var processor = new FileProcessor(readers);

    for (int i = 0; i < processor.Readers.Length; i++)
        processor.Readers[i] = null;
}

Our encapsulation has vanished, so CodeIt.Right raises a warning.

Specify CultureInfo

Here’s one you might run across somewhat frequently, especially if you manipulate strings a lot.  Perhaps you have need to compare a couple of strings with ambiguous casing.  You want to normalize them in a sense and compare apples to apples, so you call string.ToLower() on both of them.

And then you run afoul of a CodeIt.Right rule, which tells you, “specify CultureInfo.”  What does this mean?

Well, ToLower() has two overloads.  I’ve shown the first here, but the second takes a parameter called CultureInfo.  So in a sense, the warning makes your life pretty easy — just use the second overload.  But, again, what does this mean?  Why use that?

The rationale here has to do with what happens when you deploy your code beyond your own region and culture.  This also makes it doubly hard to think of for most people.  You don’t generally find yourself thinking, “What would this mean halfway around the world?”  So the tool helps remind you.

Different cultures have different takes on language, casing, date formats, and plenty of other things.  If you aren’t careful, you could find yourself facilitating comparisons or operations that don’t make sense.  What if “ToLower” has different behavior somewhere else?

I can’t tell you what to do across all cases where you might encounter this warning because it depends on where you are and where you will deploy.  But I can tell you why you’re getting it. You need a deliberate strategy on how to handle possible cultural differences.

Until Next Time

I tried to give you some eclectic concerns today, and I think I succeeded.  You got to think about what happens across different cultural boundaries, about encapsulation strategies, and about user expectation when handling errors.  Hopefully you know just a touch more about the framework than when you started.

And over the course of this CodeIt.Right series, I hope you start to internalize the rationale behind these warnings so that you can make intelligent decisions about them.