Thanks to my freelance status, I had the opportunity to work with other senior developers who are used to do a lot of code review.
I had never really done this before, so it was a new experience for me. <\/p>\n\n\n\n
So it would be a good idea to show you how I do these review, because there is a lot of talk about the benefits of code revisions, the quality of the code and its durability but not too much about the process of reviewing code .<\/p>\n\n\n\n
x<\/code>?<\/li>- Code duplication:<\/strong> I go by the \u201cthree strikes\u201d<\/a> rule. If code is copied once, it\u2019s usually okay although I don\u2019t like it. If it\u2019s copied again, it should be refactored so that the common functionality is split out.<\/li>
- Squint-test offenses<\/a>:<\/strong> If I squint my eyes, does the shape of this code look identical to other shapes? Are there patterns that might indicate other problems in the code\u2019s structure?<\/li>
- Code left in a better state than found:<\/strong> If I\u2019m changing an area of the code that\u2019s messy, it\u2019s tempting to add in a few lines and leave. I recommend going one step further and leaving the code nicer than it was found.<\/li>
- Potential bugs:<\/strong> Are there off-by-one errors? Will the loops terminate in the way we expect? Will they terminate at all?<\/li>
- Error handling:<\/strong> Are errors handled gracefully and explicitly where necessary? Have custom errors been added? If so, are they useful?<\/li>
- Efficiency:<\/strong> If there\u2019s an algorithm in the code, is it using an efficient implementation? For example, iterating over a list of keys in a dictionary is an inefficient way to locate a desired value.<\/li><\/ul>\n\n\n\n
Style<\/h3>\n\n\n\n- Method names:<\/strong> Naming things is one of the hard problems in computer science. If a method is named
get_message_queue_name<\/code> and it is actually doing something completely different like sanitizing HTML from the input, then that\u2019s an inaccurate method name. And probably a misleading function.<\/li>- Variable names:<\/strong>
foo<\/code> or bar<\/code> are probably not useful names for data structures. e<\/code> is similarly not useful when compared to exception<\/code>. Be as verbose as you need (depending on the language). Expressive variable names make it easier to understand code when we have to revisit it later.<\/li>- Function length:<\/strong> My rule of thumb is that a function should be less than 20 or so lines. If I see a method above 50, I feel it\u2019s best that it be cut into smaller pieces.<\/li>
- Class length:<\/strong> I think classes should be under about 300 lines total and ideally less than 100. It\u2019s likely that large classes can be split into separate objects, which makes it easier to understand what the class is supposed to do.<\/li>
- File length:<\/strong> For Python files, I think around 1000 lines of code is about the most we should have in one file. Anything above that is a good sign that it should be split into smaller, more focused files. As the size of a file goes up, discoverability goes down.<\/li>
- Docstrings:<\/strong> For complex methods or those with longer lists of arguments, is there a docstring explaining what each of the arguments does, if it\u2019s not obvious?<\/li>
- Commented code:<\/strong> Good idea to remove any commented out lines.<\/li>
- Number of method arguments:<\/strong> For the methods and functions, do they have 3 or fewer arguments? Greater than 3 is probably a sign that it could be grouped in a different way.<\/li>
- Readability:<\/strong> Is the code easy to understand? Do I have to pause frequently during the review to decipher it?<\/li><\/ul>\n\n\n\n
Testing<\/h3>\n\n\n\n- Test coverage:<\/strong> I like to see tests for new features. Are the tests thoughtful? Do they cover the failure conditions? Are they easy to read? How fragile are they? How big are the tests? Are they slow?<\/li>
- Testing at the right level:<\/strong> When I review tests I\u2019m also making sure that we\u2019re testing them at the right level. In other words, are we as low a level as we need to be to check the expected functionality? Gary Bernhardt recommends a ratio of 95% unit tests, 5% integration tests. I find that particularly with Django projects, it\u2019s easy to test at a high level by accident and create a slow test suite so it\u2019s important to be vigilant.<\/li>
- Number of Mocks:<\/strong> Mocking is great. Mocking everything is not great. I use a rule of thumb where if there\u2019s more than 3 mocks in a test, it should be revisited. Either the test is testing too broadly or the function is too large. Maybe it doesn\u2019t need to be tested at a unit test level and would suffice as an integration test. Either way, it\u2019s something to discuss.<\/li>
- Meets requirements:<\/strong> Usually as part of the end of a review, I\u2019ll take a look at the requirements of the story, task, or bug which the work was filed against. If it doesn\u2019t meet one of the criteria, it\u2019s better to bounce it back before it goes to QA.<\/li><\/ul>\n\n\n\n
Review your own code first<\/h2>\n\n\n\n
Before submitting my code, I will often do a git add<\/code> for the affected files \/ directories and then run a git diff --staged<\/code> to examine the changes I have not yet committed. Usually I\u2019m looking for things like:<\/p>\n\n\n\n- Did I leave a comment or TODO in?<\/li>
- Does that variable name make sense?<\/li>
- \u2026and anything else that I\u2019ve brought up above.<\/li><\/ul>\n\n\n\n
I want to make sure that I would pass my own code review first before I subject other people to it. It also stings less to get notes from yourself than from others :p<\/p>\n\n\n\n
How to handle code reviews<\/h2>\n\n\n\n
I find that the human parts of the code review offer as many challenges as reviewing the code. I\u2019m still learning how to handle this part too. Here are some approaches that have worked for me when discussing code:<\/p>\n\n\n\n
- Ask questions:<\/strong> How does this method work? If this requirement changes, what else would have to change? How could we make this more maintainable?<\/li>
- Compliment \/ reinforce good practices:<\/strong> One of the most important parts of the code review is to reward developers for growth and effort. Few things feel better than getting praise from a peer. I try to offer as many positive comments as possible.<\/li>
- Discuss in person for more detailed points:<\/strong> On occasion, a recommended architectural change might be large enough that it\u2019s easier to discuss it in person rather than in the comments. Similarly, if I\u2019m discussing a point and it goes back and forth, I\u2019ll often pick it up in person and finish out the discussion.<\/li>
- Explain reasoning:<\/strong> I find it\u2019s best both to ask if there\u2019s a better alternative and justify why I think it\u2019s worth fixing. Sometimes it can feel like the changes suggested can seem nit-picky without context or explanation.<\/li>
- Make it about the code:<\/strong> It\u2019s easy to take notes from code reviews personally, especially if we take pride in our work. It\u2019s best, I find, to make discussions about the code than about the developer. It lowers resistance and it\u2019s not about the developer anyway, it\u2019s about improving the quality of the code.<\/li>
- Suggest importance of fixes:<\/strong> I tend to offer many suggestions, not all of which need to be acted upon. Clarifying if an item is important to fix before it can be considered done is useful both for the reviewer and the reviewee. It makes the results of a review clear and actionable.<\/li><\/ul>\n\n\n\n
On mindset<\/h2>\n\n\n\n
As developers, we are responsible for making both working and maintainable code. It can be easy to defer the second part because of pressure to deliver working code. Refactoring does not change functionality by design, so don\u2019t let suggested changes discourage you. Improving the maintainability of the code can be just as important as fixing the line of code that caused the bug.<\/p>\n\n\n\n
In addition, please keep an open mind during code reviews. This is something I think everyone struggles with. I can get defensive in code reviews too, because it can feel personal when someone says code you wrote could be better.<\/p>\n\n\n\n
If the reviewer makes a suggestion, and I don\u2019t have a clear answer as to why the suggestion should not be implemented, I\u2019ll usually make the change. If the reviewer is asking a question about a line of code, it may mean that it would confuse others in the future. In addition, making the changes can help reveal larger architectural issues or bugs.<\/p>\n","protected":false},"excerpt":{"rendered":"
Thanks to my freelance status, I had the opportunity to work with other senior developers who are used to do …<\/p>\n","protected":false},"author":1,"featured_media":475,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[1],"tags":[],"_links":{"self":[{"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/posts\/474"}],"collection":[{"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=474"}],"version-history":[{"count":1,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/posts\/474\/revisions"}],"predecessor-version":[{"id":476,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/posts\/474\/revisions\/476"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=\/wp\/v2\/media\/475"}],"wp:attachment":[{"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=474"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=474"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/www.labo.mathieurella.fr\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=474"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}