71–80 of 150 posts
I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.
I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…
I think this works a lot better than trying to impose design reviews on every change. Most changes shouldn't require this kind of scrutiny; you don't want to slow down 95% of code reviews for the sake of the difficult 5%.
Earlier quoted context omitted.
What are these "more direct and optimal measures" that are cheaper than code reviews? In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net. I haven't seen Continuous integration and refactoring being thrown out of the window because of code re…
Continuous integration means you can work on multiple items at once and that you break up large feature dev into smaller chunks that get committed to master. For example, I might commit the data layer for a new big feature well before the UX, etc. Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review. What typically happens in these code re…
The idea of code reviews is that somebody else reads your code and provides feedback. Whether the reviewer is sent a pull request, a list of commits in master or a printout is a matter of implementation.
I see a cyclic problem in here: Reviews take lot of time -> developers want to get more done, not wait for a review -> so they send more changes to review -> reviews take even more time -> ...
I think the solution is to find ways to make reviews smaller instead. Breaking tasks to smaller pieces that take less time to implement and review. (But not by reducing the # of lines in diff - squeezing several changes into one commit is a sure way to make it harder to review.) Smaller reviews will also be more effective - with large reviews there's a tendency for a reviewer to wear off and just skim through the changes.
Of course that's easier to say than do... it's something I'm trying to do by myself and not fully succeeding.
Earlier quoted context omitted.
I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review. I dunno.
> the potential downsides of excessive code review What would those be?
Earlier quoted context omitted.
What are these "more direct and optimal measures" that are cheaper than code reviews? In my experience code reviews are much cheaper than other means of raising software quality. For example unit tests only start to add real value after you have written a good bunch of them, so they form this regression-safety-net. I haven't seen Continuous integration and refactoring being thrown out of the window because of code re…
Continuous integration means you can work on multiple items at once and that you break up large feature dev into smaller chunks that get committed to master. For example, I might commit the data layer for a new big feature well before the UX, etc. Code reviews mean that every time I want to integrate with master I've got to 'grab a lock' and schedule and wait for a code review. What typically happens in these code re…
Separately, minimizing diffs with the codebase is not just easier for the reviewer, it also makes 'blame' and resolving problems with existing code easier. You shouldn't break your back to minimize diffs, but as a form of hygiene it is not without merit.
Your definition of continuous integration is interesting in that I haven't seen it used in that way, or rather with that emphasis. Smaller syncs-to-master than "this is the entire feature" are great, but their effectiveness depends on automated testing, building, and deploying. When code under review is a coherent, high-quality, and tested thing, this can be a boon for CI because you get a small chunk with clear before and after states, which you might not get from just winging it. You can also work on code while other code is under review, especially with a Scrum/Kanban-type task system.
I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…
Earlier quoted context omitted.
It's not arrogance. You don't have to write perfect code to realize that the insane cost of code review is not worth the problems it is supposed to solve. Especially when code reviews are notoriously not very good anyway at excising bugs. The goal isn't perfect code. It's optimal delivery of business value. Code reviews are expensive, and not very optimal.
Why do you find them expensive? I've done thousands in my career. They don't take much time compared to actually writing the code, and adding an extra 5% of engineering time pays major dividends later without drastically reducing throughput.
When this happens, a common response is to go work on something else, but this only exacerbates the problem: now you have a bunch of commits awaiting review which are dependent in various ways. If I merge this commit I have to go and fix up that one, and that other one. My work becomes quadratic, nursing a bunch of commits along while waiting their turn through a tiny review pipeline. 5% can be 100%.
How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it eithe…
That's a management failure. The organization can't depend on a single developer being the only one to understand a module. What happens when that developer leaves, even temporarily? A competent manager will dedicate time to cross training so that at least one other team member understands everything, even if this causes a short-term productivity loss.
Earlier quoted context omitted.
Amen. Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review. A few years ago, I wor…
> Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review. Wait why are we doing code…