Welcome to my C# code-review checklist. Writing good software is a key part of any software craftspersons job. When you agree to take part in your projects code review process, you personally take on the responsibility of ensuring badly written software does not get committed into the codebase. If you are new to code reviews, it is often hard to know what you need to review and what you can let slide. This is why having a dedicated list of things to look out for while you are performing a code review can be handy. Having a standard core review checklist can help ensure consistency within your review process, hopefully, eliminating more code smells. The more rigid you can make your code review process, the less chance of maintenance fuck-ups that can occur later on 💀If you want to improve the quality of your projects, read on 🔥🔥🔥
Code Review Checklist - Essential Fixes
We will start off with the critical issues. These issues need to be resolved before allowing any code to be merged:
Duplication: DRY (don't repeat yourself) is a simple one, any new code already exists within the project needs to be refactored. DRY frequently occurs by developers who are new to a codebase and don't know about some existing code that does what they need. Fixing a DRY violation may involve refactoring code not directly related to the current PR. As long as a JIRA ticket is raised and a comment added to the PR that links to the JIRA ticket, this could be an acceptable way forward.
Good Naming Conventions : This is another big one I look for. If I can scan a bit of code and easily understand what it's doing, then, even if it's not the perfect architecture right now, when someone comes back to it in a week or month time, they can understand what it does easily. The biggest offenders of naming confusion I encounter are simplifications of names. For example, 'address can get shortened to add. What does add mean? Is it a number, is it an advert, etc... If you can read a name in a stance format, there will be no confusion. Less confusion = better quality!
Small Methods: This ties in with the single responsibility principle. In general, I think a method should be no longer than 20-25 lines max. If you don't think you can make a method that small, that's a good hint you're probably doing more than one thing in your method. From experience, bigger methods are a pain to unit tests and result in brittle tests. You can easily use the Resharper Extract method to fix these issues in seconds.
Small Classes: The same advice holds true on a class level. If you have a class that is longer than 500 lines you are probably trying to mix too many concepts in one file.
Use The Businesses Domain Language For Naming Conventions: Each company has their own vernacular of business terms. What frequently happens is the business folk name a feature 'x'. When it comes to development, a developer decides to name something 'y' as it makes more sense to them. For the rest of the project, communication between the business and the developers gets strained as people are get confused due to this mix of references. When you build a project, it's important to use the right business terminology for your classes and properties.
Open/Closed Principle: This is another SOLID principle. If I see a lot of if statements in a method, that is a warning that a pattern might be a better choice to solve the issue. If the system needs to scale later, then this code may result in lots of refactoring later. Design patterns that follow open/closed will prevent his refactoring.
Programming To Interfaces: Whenever I see a new class, always consider if it should be injected via the constructor and that it has been written implementing an interface
Has It Got An Associated Unit Test: This one is pretty obvious. Any change, new or existing, should have a unit test to prove it works
Can It Be Unit Tested Easily?: If it's agreed the method or function won't be unit tested now, the code should still be written in a way to make it easy to unit test later
Is it Guaranteed The Class Will Be Instantiated In The Correct State?: I see a lot of methods where parameters get passed in and are not checked if they are null. Over the years this has caused so many issues if it reaches production code. The easiest way to get around these issues is to use a Guard that has unit tests around it. Using a fail fast approach to error detection usually means these random object reference issues never make it to production code.
Error Handling: This will vary depending on if we're using a fail-fast or slow approach. When fast, make sure we throw exceptions, if slow, then make sure any code that might throw an exception is logged.
High Priority Issues
The issues mentioned in this section will help improve your codes readability. These issues should be fixed in order to create high-quality code:
Magic Numbers: Never allow for random meaningless numbers within your code. Over the years, I have wasted a lot of time trying to figure out what some random hardcoded number means. It might be obvious now that 17.5 is the tax amount, in the future it might not. Instead of using magic numbers refer to them in constants that describe your intentions and the problem it solves, e.g. TAX_CODE rather than hardcoding 17.5.
Constants: On all my projects, I always have a class where I store all hardcoded text. If you need to hard code a string or some text in your class, unless it's a one-off, I'll ask the developer to put it in a central location to avoid duplication.
Coding Standards: Every company has its own unique standards, single line if statements, using var etc... Having a codebase that looks like it has a consistent writer helps maintenance and troubleshooting later down the line. On every project, make sure these rules are defined and followed. Use the Resharper standard at a minimum.
Resharper Standard: Not everyone may agree with Resharpers best practice hints but for 80% of the time they are usually the most optimal. Have a policy that ReSharper needs to pass each class before it can be checked in for a review.
Typo, Spelling And Grammer: Good spelling is always important... this comes from someone with dyslexia... 'nuff said.
Comments: If I ever see a comment in code, I automatically assume the code around it is bad. If someone usually needs to write a comment, it's usually a sign the code is too difficult to read or understand. This is a very obvious sign that something needs extracting, renaming or refactoring.
Number Of Method Arguments: I think if you have more than 5 parameters being passed into a method or class, this is a sign the method/class is doing too much. In these instances, I recommended following the advice in code complete and extracting all your arguments into a configuration DTO, or, look at an alternative to split the method up.
Complicated If Statements To Bool Property?: Sometimes it can be hard to understand the purpose of an if statement. If it can't be avoided, at a minimum, extract the result of an if statement in a boolean variable. If the variable is a descriptive name that explains what the if statement is doing.
Nice-To-Have Fixes
Not all code smell issues need to be addressed in a code review, especially, in projects under time constraints. If you have some time I recommend following these practices, however, these are more nice-to-haves:
Missing Doc String: My personal preference is to not bother with docstrings. If you follow good single responsibility, good naming, small classes and methods, then docstrings are a bit obsolete if you're building an internal project.
Performance: Before we launch a project, performance deserves its own phase. Until we're in a stage in the project to measure performance, performance refactorings should be low. Obviously, if someone is iterating through a loop or doing something stupid, address it. Premature performance tweaking can be a very subjective thing. So try these types of refactoring until the end of a project
Subjective Issues: In a lot of projects, you will undoubtedly encounter subjective opinions on the best way to achieve something. If the feature or item in question is not a show stopper (99% of them usually are) I'll always favour the code that's been written, or, the quickest win to get it shipped.
Todo Comments: If a bit of code needs changing at a later date this is OK. Just make sure there is a todo next to the line of code in question, linked to a JIRA ticket so it doesn't get forgotten about!
Hope this helps. Happy Coding 🤘
