CodeIt.Right Rules Explained, Part 13

It’s time for another post in the CodeIt.Right Rules Explained series.? If this is the first such post you’ve seen, I’ll explain briefly how it works.? CodeIt.Right is an automated code review tool for .NET, and it has a bunch of rules.? In these posts, I explain those rules in detail.

Whenever I post in this series, I start with my two rules of thumb for static 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.

As always, I’ll explain that I don’t phrase it this way to be cute.? You could reduce this logically to “figure out why the tool is suggesting this.”? But I specifically opt not to.

The reason is that each time the tool confronts you with a warning, you have a choice: address the warning or ignore it.? And in either case, you should make that choice for the right reasons.? Are you ignoring it because you’ve determined that the pros of having it outweigh the cons the tool is warning you about?? Or are you ignoring it because you don’t feel like dealing with it?? The same thing applies, too, on the side of implementing fixes.

So, that said, let’s dive into three more rules today.

Avoid the Databinder.Eval Method

First up, let’s look at something from the world of web forms: the DataBinder.Eval() method.? This is a piece of code that you actually invoke from the ASP markup, like so:

<ItemTemplate>
  <asp:Label runat="server"
    Text='<%# DataBinder.Eval(Container.DataItem, "ProductID") %>'>
  </asp:Label>
</ItemTemplate>

What will happen as the framework evaluates this markup is that it will invoke DataBinder’s eval method on those two parameters in order to determine what text to show.? So it passes Eval() an object and a string, which represents a property name on that object.? It then maps that property name to the actual property and returns the property’s value.

If that sounds inefficient, it’s because it is.? This is a scheme known as late binding, which is as powerful as it is inefficient and error-prone.? In the powerful camp, it lets you do truly impressive things, like mapping GUI code to arbitrary tables in a database, sight unseen at build time.? On the flip side, it uses reflection, forcing you to pay a heavy performance penalty.? And, it’s also a lot more error-prone.? If you mistype Container.DataItem, the code won’t compile.? If you mistype “ProductID,” the code will happily compile and fail quietly at runtime.

In this case, there’s no upside to late time binding, so you should do this instead:

<ItemTemplate>
  <asp:Label runat="server"
    Text='<%# ((Product)Container.DataItem).ProductID %>'>
  </asp:Label>
</ItemTemplate>

Now you’re dealing in simple application code, which evaluates normally and doesn’t rely on inefficient reflection schemes to get you a value.

Do Not Override Operator Equals on Reference Types

Now let’s move from the GUI-heavy world of ASP to a pure application concern.? This has to do with a design guideline for .NET types.? It’s advising you not to overload the == operator for reference types.

To understand this, let’s first look at what overloading the operator might entail since there’s a decent chance this isn’t something you commonly do.? Here’s what it looks like to overload this operator.

public class Customer
{
    public string FirstName { get; set; }

    public string LastName { get; set; }

    public static bool operator ==(Customer first, Customer second)
    {
        return first.FirstName == second.FirstName && first.LastName == second.LastName;
    }

    public static bool operator !=(Customer first, Customer second)
    {
        return !(first == second);
    }

}

You’ve probably seen code that overrides the Equals() method, and CodeIt.Right has no issues with that at all.? But it will raise a warning if you write the code above.? Why is that?

Well, first of all, CodeIt.Right is, in this case, making you aware of a Microsoft guideline advising the same.? But that just backs it up one level.? Why doesn’t Microsoft want you to do it?? You can find various forms of that answer, but I’ll summarize by citing the principle of least astonishment.? What you’re doing is probably going to be really weird to those maintaining your code.

Consider that we can reason about equality in two related but subtly different ways.

1. customerOne.Equals(customerTwo) more or less asks, “Are these the same customer?”
2. customerOne == customerTwo more or less asks, “Are these customers mathematically equal?”

Notice how that second one kind of doesn’t make any sense.? What does it mean for two customers to be “mathematically equal”?? Well, by default in .NET, it means that you have two references that point to the same memory address.? But if you go overloading it, then it starts to mean something else—something unexpected and confusing.

CodeIt.Right is cautioning you about this.? You probably meant to override Equals() because this isn’t a value type and thus probably not something for which “mathematically equals” makes sense.? If it?does?make sense, then you ignore the warning.? Otherwise, you should change it to the Equals() method or otherwise re-evaluate your approach altogether.

Enforce Option Compare

In the last post in the series, I gave some love to Visual Basic, which gets far too little of it these days.? I’m going to do that again today as well.? In this case, it’s about the idea of Option Compare.

If you’ve never seen this before, I’m not entirely surprised.? It’s a fairly vestigial concern, and you have to dive deep into the project options even to mess with it.

If you’ve never touched this in your life, it will be set to “Binary.”? If you generally don’t specify it, VB assumes it to be “Binary.”? But if you go in like I did and change it to “Text,” you’ll see a warning from CodeIt.Right.? It’ll suggest that you “enforce option compare,” meaning enforce it as binary.

Why?? Well, it’s another principle of least astonishment situation.? If you go in and change this option, you’re basically opting into a scheme throughout the entire project where string comparison is case-insensitive.? So if you set this to “Text,” then the following code starts returning true instead of false.

Return "A" = "a"

I remember working with VB (and VBA) before .NET was released, and VB as a language embraces case insensitivity.? So this isn’t as insane as it seems to someone who has always worked in a case-sensitive context like C#.? But in this day and age, flipping the bit on “Text Compare” is going to seriously confuse people, and CodeIt.Right gives you a heads up.

Until Next Time

Once again, I tried to give you some variety.? We looked at another rule in the ASP world, specifically related to markup binding.? Then we took a look at a lower level design concern, using C# as the example language.? And finally, we learned a bit about the history of VB and something it lets you do.

Tune in for part 14 when we’ll take a look at yet another three rules.

Tools at your disposal

SubMain offers CodeIt.Right that easily integrates into Visual Studio for flexible and intuitive automated code review solution that works real-time, on demand, at the source control check-in or as part of your build.

About the author

Erik Dietrich

Website

I am Erik Dietrich, founder of DaedTech LLC, programmer, architect, IT management consultant, author, and technologist.

This author does not have any more posts.