Time for another post in the?CodeIt.Right Rules Explained series. In case you haven’t read any of the posts in the series, here is a short explanation.
CodeIt.Right?is an automated code review tool, which analyzes the code of your application code against a set of rules, giving you instant feedback on its quality. Each post in this series explains three of those rules.
If, on the other hand, you’re a follower of this series, nothing above is any news to you. You’ll also know that we always start the post with the following sentences:
- Never implement a suggested fix without knowing what makes it a fix.
- Never ignore a suggested fix without understanding what makes it a fix.
Nope, we’re not trying to sound clever, funny, or anything. I know it’s obvious to you that we could just say, “always understand what the tool is telling you.”
We don’t say it that way, though, and there is a reason for that. We want to make it very clear you have two options: to either implement the suggested fix or ignore the warning whatsoever. If you implement the fix, you should do so when you are fully aware of why that makes your code better. If you ignore it, you should also be aware of the dangers of doing so.
With that said, let’s get started with our first rule for today.
Specify StringComparison
If you’ve been a software developer for any reasonable length of time, it’s likely that you’ve come across the old “implicit vs. explicit” discussion. Some will argue that it’s always better to write code in as explicit a way as possible, even at the expense of brevity. Others will say that relying on the power of implicitness, despite its dangers, can reap great rewards.
You might be wondering why I started the section like this. Here’s why: the .NET Framework has somewhat of a preference for the “implicit” side of this discussion, which shows in several methods overloads in the BLC (Base Class Library).
While some of those preferences can give you great flexibility when writing code, they can also put you in a bad situation.
For example, in many instances when a culture is needed, the .NET Framework will automatically assume the current culture instead of forcing you to declare one. This can work in your favor in the cases where you do intend to use the current culture. On the other hand, it can cause you globalization problems when dealing with dates, specifically parsing and formatting.
We could say that the problem the current rule is meant to solve is a variation of the same theme. Here’s the thing: dealing with Unicode strings are hard. Like, really hard. One of the complicating factors is that the different cultures around the world have different rules about how to sort or compare text. Not all even use the same characters!
Let’s use a classic example: the famous Turkish “i.” Consider the code below:
string a = "I like chocolate"; string b = "I LIKE CHOCOLATE"; bool areEquals = a.ToUuper().Equals(b);
What should be the value in the areEquals variable at the end of that code? Well, when I run it here on my machine, areEquals is “true.” But this is not true for everyone in the world. Let’s make a small addition to the code:
Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR");
string a = "I like chocolate";
string b = "I LIKE CHOCOLATE";
bool areEquals = a.ToUpper().Equals(b);
It is indeed a small addition regarding the amount of code added since it’s just a single line. But in terms of the behavior of the program, it’s a huge difference since now the result of the comparison is false. What’s happening here?
With the line we’ve added, we’re changing the culture of the current thread in the program to the Turkish culture. And as it turns out, in Turkish, the uppercase version of the letter “i” is not? “I,” but “?.”? Caught that? In Turkish, the uppercase “i” keeps the dot. The comparison then returns false, because “I” and “?” are definitely not the same character.
To cater to this, many operations with strings provide overloads that accept a StringComparison enumeration value, which defines the culture, case, and sort rules the methods should use. Equals()?is one of those operations, so if we want the code to work, in Turkey and elsewhere, we could do this:
bool areEquals = a.Equals(b, StringComparison.InvariantCultureIgnoreCase);
With this rule, CodeIt.Right wants to nudge you in the right direction by forcing you to pick the version of the methods that is less likely to get you into trouble. It’s out of the scope of this post to do a complete treatment of globalization issues, or even best practices for strings in general. There are good resources for that out there.
When should you dismiss this rule? Basically you should do so when you’re confident that your application will only be deployed and used locally.
Avoid Prefix Enum Values With Type Name
The second rule we cover today falls into the ?naming? category. Let’s get started with an example:
public enum TaskStatus
{
TaskStatusUncompleted,
TaskStatusOverdue,
TaskStatusCompleted,
TaskStatusArchived
}
The code above is just a simple enumeration representing the possible states in a Todo application. And what’s the matter with it? Well, CodeIt.Right will complain that the type name itself is repeated in each value of the enumeration. To make CodeIt.Right happy, just rewrite the code like this:
public enum TaskStatus
{
Uncompleted,
Overdue,
Completed,
Archived
}
Some might argue that such a change is minimal, needlessly nit-picky, or even arbitrary. I beg to differ.
One point we always try to emphasize in this series is the Principle of Least Astonishment.? Good code should be pleasant to read but “boring” in the sense that it shouldn’t really surprise or shock the reader.
The convention of not including the type name in each value of the enumeration is widely known, so by following it, you’re definitely adhering to the PLA.
Besides that, by following this rule, the code on the consumer side will get cleaner since it’ll lose a lot of noise. Compare the two versions below:
var archivedTasks = taskRepository.FindByStatus(TaskStatus.TaskStatusArchived); var archivedTasks2 = taskRepository.FindByStatus(TaskStatus.Archived);
I think we can all agree that the second version looks cleaner and a lot less redundant.
Interface Should Have an Implementation
Finally, we have a rule that falls in the “design” category. This rule is, I believe, pretty straightforward. It states that you shouldn’t have interfaces in your application that are not implemented by at least a type. Easy-peasy, right? Can we finish?
Well, not so fast. I think it’d be worthwhile to talk a little bit about the justification for this rule. Why does it even exist?
The answer is that it helps validate the design of the interface. If the interface doesn’t have at least one implementation, what’s the use? It’s just dead code and you’d be better off deleting it. Perhaps the developer who originally wrote it fell prey to the interface overuse trap and started adding interfaces right and left without a proper understanding of their need. This is programming by coincidence, which is never a good sign.
And how do you fix this violation? You have just two options, really. You could either delete (or comment out) the interface or add an implementation for it.
That’s It for Today
One of the goals of this series is to showcase the variety of CodeIt.Right’s rules. Today, we started with a rule that helps you avoid globalization problems. We then covered a rule in the naming category, which will make your code more readable, less noisy, and more adherent to the principle of least astonishment.
We wrapped up the post with a rule in the “design” category, which can encourage you and your team to really think about the design decisions behind each type you create.
With that, we part ways. Stay tuned for the next installment in the series, with three more rules explained.
