# Lango team code review guideline ## What to look for in a code review (1st iteration) ### Design and architecture The most important thing to cover in a review is the overall design of the patch. - Do the interactions of various pieces of code in the patch make sense? - Does this change belong in your codebase, or in a library? - Does it integrate well with the rest of your system? - Is now a good time to add this functionality? ### Functionality Mostly, we expect developers to test patches well-enough that they work correctly by the time they get to code review. However, as the reviewer you should still be thinking about edge cases, looking for concurrency problems, and making sure that there are no bugs that you see just by reading the code. The most important questions here are: - Does this patch do what the developer intended? - Is what the developer intended good for the end-users and for other developers? You can validate the patch if you want. For changes that are hard to understand by reading the code, you can ask the developer to give you a demo of the functionality. ### Complexity A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe. Basically, you need to check that YAGNI principle is satisfied. Main questions: - Is the patch more complex than it should be? - Are individual lines too complex? - Are functions too complex? “Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.” ### Tests Ask for unit, integration, or end-to-end tests as appropriate for the change. In general, tests should be added in the same patch as the production code unless the patch is handling an emergency. Make sure that the tests in the patch are correct, sensible, and useful. Remember that tests are also code that has to be maintained. Don’t accept complexity in tests just because they aren’t part of the main binary. Main questions: - Will the tests actually fail when the code is broken? - If the code changes beneath them, will they start producing false positives? (Fragileness) - Does each test make simple and useful assertions? - Are the tests separated appropriately between different test methods? ## What to look for in a code review (2nd iteration) ### Naming Pretty self-explanatory. A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read. ### Comments Look at comments that were there before this patch. Maybe there is a TODO that can be removed now, a comment advising against this change being made, etc. Main questions: - Did the developer write clear comments in understandable English? - Are all of the comments actually necessary? - Are comments useful? - Do they explain why some code exists, not what it does? ### Style Make sure the patch follows the appropriate style guides. If you want to improve some style point that isn’t in the style guide, prefix your comment with “Nit:” to let the developer know that it’s a nitpick that you think would improve the code but isn’t mandatory. Don’t block patches from being submitted based only on personal style preferences. ### Documentation If documentation/changelogs is missing, when it should be present, ask for it. ### Every line In the general case, look at every line of code that you have been assigned to review. If it’s too hard for you to read the code and this is slowing down the review, then you should let the developer know that and wait for them to clarify it before you try to review it. If you can’t understand the code, it’s very likely that other developers won’t either. So you’re also helping future developers understand this code, when you ask to clarify it. If you understand the code but you don’t feel qualified to do some part of the review, make sure there is a reviewer on the patch who is qualified, particularly for complex issues. ### Context It is often helpful to look at the patch in a broad context. Sometimes you have to look at the whole file to be sure that the change actually makes sense. For example, you might see only four new lines being added, but when you look at the whole file, you see those four lines are in a 50-line method that now really needs to be broken up into smaller methods. ## Speed of the code review process When code reviews are slow, several things happen: - The velocity of the team as a whole decreases. Yes, the individual who doesn’t respond quickly to the review gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each patch waits for review and re-review. - Developers start to protest the code review process. If a reviewer only responds every few weeks, but requests major changes to the CL each time, that can be frustrating and difficult for developers. - Code health can be impacted.Slow reviews discourage code cleanups, refactorings, and further improvements to existing patches. If you are not in the middle of a focused task, you should do a code review shortly after it comes in. One week is the maximum time it should take to respond to a code review request. ### Speed vs Interruption There is one time where the consideration of personal velocity trumps team velocity. If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review. Instead, wait for a break point in your work before you respond to a request for review. This could be when your current coding task is completed, after lunch, returning from a meeting, coming back from the breakroom, etc. If you are too busy to do a full review on a patch when it comes in, you can still send a quick response that lets the developer know when you will get to it, suggest other reviewers who might be able to respond more quickly, or provide some initial broad comments. **None of this means you should interrupt coding even to send a response like this.** Send the response at a reasonable break point in your work. ### LGTM with comments In order to speed up code reviews, there are certain situations in which a reviewer should give LGTM even though they are also leaving unresolved comments on the patch. This is done when either: - The reviewer is confident that the developer will appropriately address all the reviewer’s remaining comments. - The remaining changes are minor and don’t have to be done by the developer. ## Tracking According to the established procedure, all patches under review should be added to the "Lango team reviews" board. Accordingly, responsible people on the side from which actions are expected should be assigned. In the table with patches for review, a new field has appeared - "No response since". Currently, it needs to be filled in manually when updating the task status (for example, after sending the changes for review and waiting for feedback). Additionally, a "Rotten PRs" board has been introduced, which automatically includes tasks that have had no activity for more than a week. Those tasks should have maximum priority on the todo list of people assigned. Otherwise, they need to specify an exact reason for postponing.