You know what I hate more than code review? Shipping bugs. The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Similarly, when acting as the code r…
Code review doesn’t catch bugs. Sure, sometimes an experienced dev can spot a bounds error or race condition, but CRS are for knowledge sharing and style adherence.
Confessions of a programmer: I hate code review (2010)
91–100 of 165 posts
Re: Confessions of a programmer: I hate code review (2010)
#92Re: Confessions of a programmer: I hate code review (2010)
#93Some simple tips I use to make this better. 1) Anything cosmetic gets a "nit" added to the comment. 2) When a review looks like mostly assets or boiler plate, I look through quickly for anything glaring and approve while noting my assumptions that the author has run the build and verified the additions. 3) If I'm in a hurry and the author is available I grab them and do an over the shoulder review. You might be surpr…
Re: Confessions of a programmer: I hate code review (2010)
#94In one of the teams I worked in we had a policy to mitigate some of the things we found inefficient in our code reviews: 1- anything done in a pair doesn't need a code review. 2- any story that is more than 3 story points should be worked on in pair. 3- if you are doing a code review and you find something that needs to change you don't leave a comment, you update the branch and ask the original branch owner to revie…
Conversely,I could see that requirement causing particularly poorly-skilled developers tying up a disproportionate amount of the more skilled developers' time
Re: Confessions of a programmer: I hate code review (2010)
#95Only time i've come to hate code review was when my counterpart (reviewer or the author) has been overly pedantic with no hint of any empathy in hir writing. I do not know was it because of inherent insecurity or no previous experience writing in more friendly manner but boy oh boy i was boiling inside arguing about useless things. The socially awkward seem to be in high numbers amongst programmers which makes these…
Why are you looking for empathy in code reviews?
Why are you getting feelings involved at all?
Re: Confessions of a programmer: I hate code review (2010)
#96My biggest problems with code review are:
1. The reviewer usually lacks the context to truly review the solution and all its implications, and instead gives only a cursory review focused on style and trivialities. Sometimes they're familiar enough with the code to give an insightful review, and there's benefit to commenting on even the small things, but I've seen countless major architectural problems pass review because the reviewer lacked the time and familiarity to notice them. This isn't helped by reviews typically being done on GitHub code diffs; you're seeing a couple out of context lines and it's hard to understand how it all fits together.
2. By the time code is ready for review, it's too late. Sure, you can stop the presses if you notice an egregious problem (not that anyone usually does; see #1). My team tries to keep its reviews small and frequent, but they still often contain one or more full days of effort. If the solution has major problems, catching them after the fact means a lot of wasted effort on rewrites. Often there's no time to fix it right, so someone hacks around the faults and ships a "good enough" product. The tech debt piles up quickly.
As a result of both of these, code reviews are often little more than a rubber stamp.
This isn't to say that I don't think code reviews have any merit. I think that code reviews on every single change are a waste of time. Periodic detailed reviews of architecture are valuable, and periodic team reviews of code can help spread good patterns.
Many of these problems are obviated by pair programming. It's real-time code review before it's too late. It's the single biggest benefit to pairing, although there are plenty of others (like sharing knowledge and building team bonds). Pairs are also subject to the same pitfalls, but at a far lower rate than solo developers.
Re: Confessions of a programmer: I hate code review (2010)
#97Some simple tips I use to make this better. 1) Anything cosmetic gets a "nit" added to the comment. 2) When a review looks like mostly assets or boiler plate, I look through quickly for anything glaring and approve while noting my assumptions that the author has run the build and verified the additions. 3) If I'm in a hurry and the author is available I grab them and do an over the shoulder review. You might be surpr…
These are all good tips, but I want to stress the importance of (6). Much of what the original post is talking about can be improved by adopting the habit of making small, incremental changes. As the author, you no longer have to wait "in the order of weeks" for the feedback and a potential rebase should pose little risk. As the reviewer, you don't have as much mental burden and most reviews can be done "in-between"…
Specifically, when doing something where the solution is well known for the beginning, creating PRs which are small and easy to understand is easy.
When instead I have to solve difficult problems for which I don't already know the solution beforehand, and which require writing quite a bit of complex code - eg multiple classes that work with each other, and which maybe work with some complex external system I/the team doesn't already know well - I find it difficult to split the PR into smaller chunks, because I "evolve" the separate classes together, while also doing exploratory testing.
"Production ready" code comes later in the process, and at that point I usually have quite a bit of code. I noticed that people don't like reviewing the non-production-ready code, but what's the alternative? In theory, after I found out what the solution is, I could go back and rewrite it better from the beginning, but that would just require too much time.
Didn't find a good answer yet.
Re: Confessions of a programmer: I hate code review (2010)
#98That said, I think that there are some prerequisites for productive code review. Changes are preferably small, reviews need to happen in a timely fashion, the surface area for style nitpicking needs to be minimized with linting and formatting tools and the team needs to be pretty accepting when it comes to different minor design decisions that the different team members might make.
If your change is 2000 lines for what could have been several atomic changes and you get 50 comments about indentation, "you should do it this [slightly different way that the reviewer would have preferred that doesn't significantly improve legibility or design]" after prodding the other team members for a week, address the comments and wait yet another week (and also manually rebase the patch because of 10 conflicts) it's no fun.
Re: Confessions of a programmer: I hate code review (2010)
#99I love code-reviews. With the right people, they can be useful both to achieve high-standards of code quality and also improve your own engineering process. The requirements are quite high, I think you need some combination of the following: 1. Both reviewer and reviewee are focusing on getting the best outcome possible, in good faith and with generosity. 2. The reviewer concedes that there can be equally valid appro…
I think in such a situation, receiving a review is great, even though the downsides mentioned in the article ("what am I going to do while I wait for the review to come in?") are still presents. However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doi…
I'm sure there are people that feel exactly the opposite, but it's worth trying.
Re: Confessions of a programmer: I hate code review (2010)
#100In one of the teams I worked in we had a policy to mitigate some of the things we found inefficient in our code reviews: 1- anything done in a pair doesn't need a code review. 2- any story that is more than 3 story points should be worked on in pair. 3- if you are doing a code review and you find something that needs to change you don't leave a comment, you update the branch and ask the original branch owner to revie…
#3 sounds like a really good idea to get the reviewer to put their money where their mouth is, so to speak. Conversely,I could see that requirement causing particularly poorly-skilled developers tying up a disproportionate amount of the more skilled developers' time
anyway duo to #1 and #2 what actually ends up needing a review is the only minor or medium scope changes, and most of the time the commit will be renaming a variable/method or adding a test case.
of course if you see the PR went in the wrong direction you can still reject it.