<story><title>Code reviews do find bugs</title><url>https://two-wrongs.com/code-reviews-do-find-bugs.html</url></story><parent_chain><item><author>jellyfishbeaver</author><text>I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions?&lt;p&gt;- The same people leave detailed comments on others&amp;#x27; merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs.&lt;p&gt;- People blindly accept suggestions with no resistance or discussion to get the review over with.&lt;p&gt;- People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I&amp;#x27;ve had to revert several of these).</text></item><item><author>willio58</author><text>Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users.&lt;p&gt;What I find more though is code that isn&amp;#x27;t thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find.&lt;p&gt;Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it!&lt;p&gt;One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict.&lt;p&gt;Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever</text></item></parent_chain><comment><author>ecshafer</author><text>Good PR culture is definitely something that has to be built from the ground up, and supported top down. At Shopify, who I think has a really good PR culture we have a few things that I think help (beyond a good CICD, and static analysis tools):&lt;p&gt;1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expected to get 2 reviewers from our team to okay.&lt;p&gt;2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc.&lt;p&gt;3. Some areas have code owners that might require an additional review from a specific team.&lt;p&gt;4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases.&lt;p&gt;I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.</text></comment>
<story><title>Code reviews do find bugs</title><url>https://two-wrongs.com/code-reviews-do-find-bugs.html</url></story><parent_chain><item><author>jellyfishbeaver</author><text>I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions?&lt;p&gt;- The same people leave detailed comments on others&amp;#x27; merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs.&lt;p&gt;- People blindly accept suggestions with no resistance or discussion to get the review over with.&lt;p&gt;- People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I&amp;#x27;ve had to revert several of these).</text></item><item><author>willio58</author><text>Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users.&lt;p&gt;What I find more though is code that isn&amp;#x27;t thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find.&lt;p&gt;Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it!&lt;p&gt;One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict.&lt;p&gt;Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever</text></item></parent_chain><comment><author>xmprt</author><text>Culture for code reviews doesn&amp;#x27;t start out of thin air. Unless you have processes for CI&amp;#x2F;CD, testing, task estimation, retrospectives, incident postmortems, etc., there&amp;#x27;s never going to be a point where you will convince people that they&amp;#x27;re helpful. So start with those.&lt;p&gt;There&amp;#x27;s always going to be pushback from adding more process, but if there&amp;#x27;s an understanding amongst the team that keeping things working is P0 then these processes will slowly&amp;#x2F;naturally come up as the team realizes that investing in them proactively will save them time down the road.</text></comment>