CodeReviewGuidelines.md

Code Review Guidelines

These are just what it says: guidelines. When reviewing code, this is the vague problem space of issues to watch for. Every guideline may have exceptions, for author and reviewer to discuss and come to an agreement on.

Function

• Does the code compile?
• Have unavoidable warnings been suppressed and explained in comments?
• Does the code have passing tests?
• Do the unit tests actually test all of the business logic, including edge cases?
• Does the code accomplish the business requirements (or fix the bug)? Does it only accomplish the requirements (no extra functionality)?
• Is the code thread-safe? Does it handle threading properly when accessing resources?

Security

• Is data/system access controlled as defined by team policies? (E.g. checking for appropriate user authentication/roles)
• Is user input sanitized/escaped? E.g., are SQL inserts performed using secure tools such as ORM/prepared statements? Or is user input blindly appended to SQL command strings, allowing SQL injection attacks? What about preventing malicious HTML/CSS/JS from being displayed in the web UI?
• Is user input validated as soon as possible, and stored in appropriate domain objects immediately? If a URL query parameter is a number, immediately validate that it is within the expected range, and store it as an Integer or Double, not as a String. Similarly, if a variable is a comma-delimited String of values, immediately split() it into a List; don't leave it as a String to be re-handled in multiple places downstream.
• Is user input validated both in the client and on the server? Validate in the client to improve the user experience (faster response time if they goofed; fewer network calls), and validate on the server for security and sanity.

Comments

• Do all comments make sense? A comment should provide information which the code does not. Comments in this form provide no extra information:

   //Connection object
   Connection conn = null;

• Comments required for understanding the code should be minimal; the code should generally be clear enough to understand without needing explanation. Exceptions exist; e.g., details of formulas used or RFCs implemented.
• Have all comments been updated to match new changes? Out-of-date comments are worse than no comments at all.
• // end methodName at the end of a method is a code smell -- it likely signifies that a method is too long. If its functionality is reduced and broken down, that comment is no longer needed.
• Has commented-out code been removed? It clutters the codebase, and can be retrieved via version control if needed in the future.

Structure/Style

• Is the code formatted according to team standards? (Can be set up as a save action in some IDEs)
• Does each function fit on one screen in your IDE (under ~50 lines)? Large functions are harder to understand fully.
• Is each code file under ~1000 lines? Large code files likely imply the class is doing too much.
• Is indentation within each method kept to 1 or at most 2 levels? (eg, avoiding excessive if/else/for nesting - break deeper layers out into other methods with clear names, or use appropriate patterns to handle complex control needs)
• Does each method do only what it is expected to do? Does the method signature communicate this clearly?
• Is code reused where possible, via services or inheritance? Is code placed in a method or class for re-use, rather than copied wholesale to each place it is needed?
• Is the code simple and readable? IE, is there a much simpler way of accomplishing the same thing, or a level of nesting that can be removed? Did the author create a new service where an existing one (or an open-source library) could have sufficed?
• Are custom classes used for complex data structures? IE, a List<MyDataObject> structure is far preferable to a String[] array with a comment indicating the data definition -- the class is more maintainable and far easier to test and review and reuse than the array. (Yes, I have seen this in production.)
• Code to interfaces whenever possible. IE, declare List instead of ArrayList, Map instead of HashMap. This assists in dependency injection -- let the calling process or the service you are referencing decide which particular implementation is best. This also simplifies testing by making it easier to create and use mock objects.
• Are variables, classes, etc., given descriptive, full-word names whenever possible? If you shorten requisition to r in a SQL query, it makes it impossible to easily see everywhere that reference is used. In most modern environments, there is no need to shorten requisition to req. Using the full word eliminates any doubt as to its use, as well as any confusion due to inconsistent abbreviations (such as impAgency, implAgency, impAgent, etc.
• Is the language’s style guide followed? Consistency is key for helping other developers follow the code. Using the language-specific style guide (vs using your personal preference in every language) makes onboarding easier for new hires who are already familiar with that language.