Welcome back to the CodeIt.Right Rules Explained series. For those of you who haven’t seen an installment in this series, let’s explain what it’s all about.?CodeIt.Right?is a tool that performs automated code review for .NET. It checks your code against a set of rules, giving you valuable feedback on its quality. Throughout the series, we’ve been explaining these rules, always three at a time.
In every post in this series, we start with the following two rules of thumb:
- Never implement a suggested fix without knowing what makes it a fix.
- Never ignore a suggested fix without understanding what makes it a fix.
Trust me?we don’t phrase it this way to sound funny or anything. It’d be the same thing to say ?figure out why the tool is suggesting this.? But we don’t. And here’s why.
Each time CodeIt.Right shows you a warning, you must decide to either address it or ignore it.?Make that choice for the right reasons. Ignore the warning when you’re truly convinced it doesn’t apply to your situation or doesn’t provide enough benefits. Don’t ignore it because you don’t understand it very well and don’t feel like handling it.
Now that you understand what this post is all about, let’s get to today’s three rules.
Remove Unused Private Methods
The first rule in today’s post is “remove unused private methods.” This rule is exactly what its name suggests: it wants you to remove from your code those private methods that don’t have any callers. But why?
First of all, you need to know that, with this rule, CodeIt.Right is actually endorsing a Microsoft guideline.?But why does Microsoft want you to delete unused private methods? For starters, there’s the obvious reason that classes with fewer methods are easier to navigate and maintain. Having lots and lots of useless methods lying around isn’t exactly the best path to clean code.
There’s also an argument of safety to be made. If the developer creates one or more private methods and then abandons them, there’s a reason for that. And the reason might simply be that the developer found out the code in the method was wrong (in which case they should’ve deleted the offending method). Keeping useless but wrong methods around opens the possibility for other unsuspecting developers to call a method and make use of its inaccurate results.
But the main reason for removing unused private methods has to do with the category to which this rule belongs: performance. All methods, private or not, used or unused, add to the overall assembly size, which can slow down the application.
There’s one important point you must remember?about this rule and similar ones (that is, rules that detect unused code). They might be costly in regard to analysis time. If you have a large codebase, create a new, separate analysis profile for enabling this rule?so it doesn’t impact your normal analysis.
A Collection Class Name Must Have a “Collection” Suffix
Today’s second rule falls into the “naming” category of CodeIt.Right rules. It says you should include the word “collection” in the name of your collection classes. Why is that a thing?
First, as with the previous rule, this one also makes you aware of a Microsoft guideline.?Again we ask, why does Microsoft want you to do this?
To understand the reasons for this advice, we must first make clear what’s meant by “collection class.” Is any class that represents “a bunch of stuff” a collection class? Well, not really. Consider the code below:
public class Team
{
public string Name { get; }
public IEnumerable<Player> Roster { get; }
public Team(string name, IEnumerable<Player> roster)
{
Name = name;
Roster = roster;
}
}
The class above models a sports team. It uses an IEnumerable of instances of the player class to model the roster. Fair enough. But then, some developer read somewhere that they should create a whole new class to model the player roster. So, they do that. Here’s what they come up with:
public class Roster : IEnumerable<Player>
{
public Roster(IEnumerable<Player> players)
{
Players = players;
}
public IEnumerable<Player> Players { get; }
public IEnumerator<Player> GetEnumerator() => Players.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => Players.GetEnumerator();
}
What’s the point of implementing such a class? They didn’t get any new functionality that they didn’t have with a plain old IEnumerable of players. This class isn’t a proper collection. A collection class is one that implements the ICollection or the ICollection<T> interfaces. Here’s an implementation of the roster as a collection:
public class Roster : ICollection<Player>
{
private List<Player> innerList = new List<Player>();
public Roster(params Player[] players)
{
innerList.AddRange(players);
}
public int Count => innerList.Count;
public bool IsReadOnly => false;
public void Add(Player item)
{
innerList.Add(item);
}
public void Clear()
{
innerList.Clear();
}
public bool Contains(Player item) => innerList.Contains(item);
public void CopyTo(Player[] array, int arrayIndex) => innerList.CopyTo(array, arrayIndex);
public IEnumerator<Player> GetEnumerator() => innerList.GetEnumerator();
public bool Remove(Player item) => innerList.Remove(item);
IEnumerator IEnumerable.GetEnumerator() => innerList.GetEnumerator();
}
This implementation is, again, very simple. It just delegates every method and property calls to an inner list. As you can see, by implementing the ICollection<T> interface, you must provide methods that allow the consumer to manipulate the collection in several ways, not just enumerate them.
But the implementation above triggers a violation of the rule we’re covering in this section. The name of the class doesn’t contain the word “collection.” Why is this such a big deal after all? Well, the answer lies with the famous “principle of least astonishment.” When you name your classes, methods, and general software artifacts in ways that follow established conventions, your code becomes more predictable and, by consequence, easier to understand and maintain.
To solve the violation?no surprises?just include the word “collection” in the name of the class. Sure, you could rename the class to “RosterCollection” but that wouldn’t make much sense. The usual practice here is that the name of the class should be the name of the item it collects. So, the better name here would be “PlayerCollection.”
A Source File Name Should Match Public Type Name
Finally, our last rule for today’s post belongs to the “general” category. The rationale for this rule, like the previous one, draws inspiration from the principle of least astonishment. It states that the name of the source code file should match the name of the public type declared in such a file. So, if you have a class called “BookAuthor,” name the file containing it “BookAuthor.cs.” But why is CodeIt.Right bothering you with this?
As it turns out, we have a very simple answer for that. Having different names for a type and the file in which it resides creates unnecessary confusion.
Specifically, maintainers might struggle to navigate the codebase. Sure, each version of Visual Studio gets better navigation features, allowing the developer to reach any type in a matter of seconds. But think about trying to find a class on GitHub’s web interface. It’s way easier to find the “Product” class if its file is named “Product.cs” instead of “File3.cs”, right?
Until Next Time
Today we’ve covered another three rules of CodeIt.Right. The first falls into the performance category, but you could make the argument it leads to cleaner code and easier maintenance. The other two encourage you to write code that’s more predictable and easier to understand, navigate, and maintain.
All in all, I think it’d be fair to say that today’s theme was “ease of maintenance,” even though it wasn’t necessarily planned that way. Until next time?when we’ll have?three more rules for you and (hopefully) another unifying theme!
Learn more how CodeIt.Right can help you automate code reviews and improve the quality of your code.
