I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value.
>The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO).
Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.
I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.
For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
We have since enabled post-push linting on that repo, but I think that had someone else looked over it, they may have spotted that issue before it was merged.
> Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done.
To some extent, you're correct, but only if the code base is relatively static and conventions don't change. That usually doesn't hold true over time.
I agree with you, but it would seem rare. Maybe 1 out of 50 code reviews.
>For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.
I would think in this example, the developer didn't bother to test his own code and he is using the code review as a replacement for that, which seems lazy. I would imagine the config error would pop up when he was running the application and that the config line he wrote was directly used by his changes. It would certainly pop up during QA.
> 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia.
This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review.
On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to).
> 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine.
I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed.
Edit: s/submitted/submitter/