# Code Review ## Connection - 10' * Describe what are your objectives when you review peer’s code ? * How do you do it ? * Share in pair ## Concepts - 10' ![](/files/-MRkKkzzgwUpde5fylBh) ### How to do it ? * Don't review anything that we can automate * Spell check * Code beautifier * Small batches * No more than 200 to 400 lines of code (LOC) at a time * Brain cannot process and detect errors if too much data * At least two developers sitting together * Junior Dev should pair with a more experienced one * Whole team when the feature is risky or complex ### Who ? * Everyone code can be reviewed * Any Dev can review * The code of the most senior is also reviewed * The reviewer * should be : * Humble * Give feedback * Don’t blame * Need to get knowledge of the functional analysis * Checkout the code on his/her workstation and run it ### When ? * Could be on-demand * Or, if the CI quality gate not passed * And anyway, before a story can be considered “Done” ### Why ? * Better code * Ensure code meets standards * Find bugs * Ensure code does what it’s supposed to * Check code is understandable * Share Knowledge * Spread code Ownership * No better way to learn the right practices and promote the company’s code guidelines and requirements * Collaborate on Design * Improve communication inside the team * Every team members talk to each others * Foster companionship and improve team cohesion * Motivate to progress * If you keep in mind that your code will be reviewed by some colleagues, you will put more care into it * You will take the time to properly : * Name identifiers * Write tests * Introduce well-fitted abstractions * Use carefully the enterprise wide patterns ### Different Workflows #### [Gateway review](https://blog.jetbrains.com/upsource/2017/01/18/code-review-as-a-gateway/) A developer : * Picks up or is assigned a task * Implements the code to the best of his/her ability * Submits it for review The reviewer checks the code and, if everything is OK : the code is merged into the main development branch. ![](/files/-MRkNV1kZ_ri5_VIG--a) #### [Knowledge Sharing](https://blog.jetbrains.com/upsource/2017/03/14/code-review-for-knowledge-sharing/) * Learning at the center of the process * The purpose is not to check if the code is correct * But to share with other developers what changes have been made ![](/files/-MRkNhQecfPERGvn8wjQ) #### Early Design Feedback * A collaborative and iterative approach to * Check the code frequently while it’s being built * So design feedback comes at the right time ![](/files/-MRkNwoTTCzQGC9hgGvI) ### Anti-patterns (List from Trisha Gee) * My job as a Reviewer is to find problem * When you try to find problems, you will always find more problems * Always remember that you bring you own preferences / biases * Nit picking * Spacing is wrong / Formatting * Automate this kind of check * Waste of time for a human * Design changes when the code works * You find it too late * Design must be discussed early on * Inconsistent feedback * Feedback biased because I have followed a security training * Next week it will be on compliance for example… * To avoid this : * Have clear guidelines * Use checklists * The Ghost Reviewer * No time for it * ***The bigger the code review, the more likely it will receive a green check*** * Ping Pong Reviews (Between reviewer and author) * Happens when you goal is to find problems * Have a Definition of Done * Clear guidelines ### Use a checklist ![](/files/-MRkLOpqlYxfaHbEGBaX) #### Example * [ ] **Compliance** * [ ] With internal tech radar (Lombok, MapStruct, ..) * [ ] Right version of the libraries (Java 8) * [ ] Pass internal automatic quality gates * [ ] **Operability** * [ ] Exceptions are easily understood * [ ] Logging give context for debugging * [ ] Configuration is separated from the code * [ ] Is the code **readable** * [ ] The names covey **intent** * [ ] APIs are documented * [ ] Is the code **SOLID** ? * [ ] Team **conventions** are followed * [ ] There is **no obsolete code**/function * [ ] There is **no commented out code** or TODOs * [ ] Use of design patterns when appropriate (modularity, simplicity, ..) * [ ] Is the code testable ? * [ ] Do the tests * [ ] Cover requirements * [ ] Cover confusing / complicated and limits of the code * [ ] Checks the performance ## Concrete Practice - Code vengers 30' * Form groups of 3 * Each team member take : * A role card : ***keep it secret*** * A checklist * A code sample * 5’ in solo : * Prepare a code review as a code-venger : role-play based on your role card. * Review time : * Each member explains his/her discovery in the code to the two others. * They have to guess who is the code-venger * And keep note how they feel during the review #### Here are the cards ![](/files/-MRkRDUR-dSzyWWGZ0qP) {% file src="/files/-MRkRXJxwCgqlt50V-pt" %} Download pdf cards here {% endfile %} {% file src="/files/-MRkSrGmloFbGtIzwPRJ" %} Code samples for code reviews {% endfile %} ### Debriefing round 1 : * Who was who in your group ? * How did you feel during feedback session ? #### Explain things to avoid during code review ![](/files/-MRkT3E1EXgXTJ8KWXZV) * Judgmental questions : * Avoid to say : Why didn’t you just do \_\_\_ here? * Try it : What do you think about \_\_\_, which has the benefit of\_\_\_\_? * Being sarcastic * Avoid to say : Did you even test this code before you checked it in * Try it : This breaks when \_\_\_. Can you please address this case? * Passing off opinion as fact * Avoid to say : This component should be \_\_\_. * Try it : Since this component \_\_\_, it could be made \_\_\_. This will improve\_\_\_ ### Lets do a second round * Prepare once again the review of the code by being yourself * Try to avoid behavior sentences explained before ## Conclusion - 10' * Bootstrap your team check-list * What / When to review ? #### Don’t let code reviews be an ego match {% hint style="success" %} ***“Being the most senior person on the team does not imply that your code does not need review\...”*** {% endhint %} ### Resources * [Code Review Best Practices by Trisha Gee](https://blog.jetbrains.com/upsource/2018/08/30/code-review-best-practices/) * [The Code Review Pyramid](https://www.morling.dev/blog/the-code-review-pyramid/) * [Async Code Reviews Are Killing Your Company’s Throughput](https://www.slideshare.net/kobac/async-code-reviews-are-killing-your-companys-throughput-248758692) * [Code Review Worflows](https://blog.jetbrains.com/upsource/tag/code-review-workflows/) * [Code Review Guidelines by Patrick Smacchia](https://blog.ndepend.com/what-is-code-review-guidelines-best-practices/) * [Ebook "What to look for in a code review"](http://jb.gg/book/codereview) * [Video](https://youtu.be/3pth05Rgr8U) --- # Agent Instructions: Querying This Documentation If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question. Perform an HTTP GET request on the current page URL with the `ask` query parameter: ``` GET https://yoan-thirion.gitbook.io/knowledge-base/software-craftsmanship/practices/code-review.md?ask= ``` The question should be specific, self-contained, and written in natural language. The response will contain a direct answer to the question and relevant excerpts and sources from the documentation. Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.