Sometimes going nuclear on code is a mistake, but finding people who have the skill and patience to look at existing code (good or bad) is really difficult. Frankly finding someone who has an ability to gain mastery of others code should be seen as a much more valuable skill than simply writing new code, but in truth we rarely value that skill.
With all that said if reading bad code is a skill, improving on bad code is a virtue! Identifying code smells and eliminating technical debt is a part of the development cycle that is rarely accounted for or even measured. Most developers I work with know how to strategically manage new technical debt. However, eliminating debt in existing code comes with added responsibility to test coverage and resource/time allocation that require executive approval.
So here are the most common issues (in order) I find in code reviews :
Twin (duplicate) code
This is an obvious one, and you should rarely see this pattern within a single class or C# page. I find this pattern tends to rear its head across multiple classes and namespaces, where someone needs to take a step back and use tools that find these patterns. I tend to warn people “if you use copy and past on an entire method you are probably doing it wrong”.
Walking dead code
This one is pretty obvious but we should always be on the look out for redundant and unused code. I have seen entire pages that harbor code just for future reference even though there is first class source code repository in use. Trust your source code system, delete unused code!
James and the giant Class
I really believe in the idea of single responsibility principle, make your classes very specific and succinct. So if you have thirty methods in a single class you may want to check if it is suffering from some unnecessary bloat. Also I tend to get a little nervous when I see your class creating more than a couple instances of other classes.
Breaking the Prime Directive
Primitive type abuse is always a concern in code, using primitive types is simple and straight forward, however, there are many cases where we need to guard against a range, or it may require some kind of validation (credit card, social security numbers, age, etc.) and a simple primitive type will not cover it.
Inception
This is the kind of code laden with if/else statements, and interwoven with multiple switch statements. I have seen a single if statements make frequent use of multiple not conditions, and it takes even the most logical thinker a dozen minutes to consider the possible paths through the code. Using a metric like Cyclomatic complexity can help identify code like this, but I find it can produce a lot of false positives especially for switch statements.
Independence day
It is time to set your classes free, along with reducing the overall size of your class you should be working to ensure that each class does not have unnecessary dependence on another class. When classes rely on specific features of other classes it tends to ensure that a simple change in one class arbitrarily affecting another class.
The Method with no name
I find very few people who naturally write good comments, they are usually just rewording the method name (e.g. GetFileList() will have a comment like “This gets the file list” o_O). If your method is sufficiently complex that the name does not capture it, remember to comment on the “why” rather than the “how”.
I have found that making technical debt visual helps people make more decisive decisions about how to combat it (using tools to measure complexity or define styles). You have to do your best to highlight the churn that is produced by having to read bad code, it also helps to stop referring to things as “the right way vs. the fast way” and call it what it is,“the right vs. the wrong way”.
Comments are closed.