<i>"Approving a pull request without even testing the code it’s very dangerous"</i><p>Well; code review is not QA. My approval means - I'm OK with how the code layer is knitted. It doesn't mean I've tested the changes.<p>Of course, if there's some logical problem with the implementation (such as the author seemingly failing to handle an edge case), any careful reviewer should catch this out.<p>Still, this is a situation where the overlook could be detected through theoretical code analysis, so to speak: which is what a review is. That's not testing.<p><i>"some 2~3 line changes may not need to be tested, but those are the exception, not the rule."</i><p>Generally speaking everything needs to be tested (not necessarily manually), but that's beyond the point. Testing is not reviewing, and vice versa.
I also find code review to be an <i>exceptionally</i> effective way of learning a new language, especially languages that have a different mental model from languages you’re already familiar with. I do some Rust training and mentorship, and have found <i>the</i> most effective ways of teaching (once past the basics) to be reviewing their code, giving suggestions on why this pattern might not scale well in Rust, how you can take advantage of ownership here, how to be pals with the borrow checker rather than cloning stuff all over the place there, how expression orientation can make your code neater and more readable, how iterator chaining could help you play to Rust’s strengths, how to work with rather than against Result-based error handling, why you should really give up trying to implement something in an inheritancy way and what an alternative design might be, <i>&c. &c.</i>
I find post-coding, pre-merge code reviews ineffective, for a change.
The code was already written. Time/money spent. Finding out that solution is subpar at this stage is costly, and delaying integration makes ineffective teams.<p>If you want to enforce standards, automate it (there are multiple highly configurable tools for that).
If you want good solutions, pair program.
If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor. Use TDD / BDD to ensure the app still works.
I've observed a small (yet growing) sentiment emerging lately that code reviews aren't all they're cracked up to be, and can actually be a net negative. I'm somewhat on the fence and can see all the arguments, though still err on the side of code review. But I am somewhat convinced that the way most teams (at least those i've seen) practice them isn't ideal.
Just asking the person who wrote the code to explain it better so I could properly review it can be helpful. I've had cases where this led to "yeah, that's not right, I'll redo it" even if I had no clue what was going on.
Just to be clear - I am strongly in favor of code reviews and do them every day. However, the article advocates spending 20-40% time on doing code reviews. Good luck convincing your manager why your productivity is so low when others are churning out high quality features (because you spent 20-40% of your time reviewing others' code but they didn't spend commensurate time reviewing yours). Net result - other people's code is higher quality, they get more done. Your code is lower quality and you get less done. Good luck getting that promotion or even retaining your job.
Almost everyone agrees that code review is important and worth spending time on, so why don't we think much about our code review tools? Most of us just accept whatever GitHub gives us (esp. smaller or newer teams)<p>I'm building CodeApprove (<a href="https://codeapprove.com" rel="nofollow">https://codeapprove.com</a>) to be a much more powerful, enjoyable, and efficient code review tools for teams on GitHub. If you're interested in trying our Alpha email me.
I must be the exception, but I've only had bad or really "meh" experiences with code reviews:<p>Doing the reviews myself:<p>A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs.<p>if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case<p>There's nothing to review, really, I don't know the business case, the code looks okish.<p>I really don't care about unused imports (unless you make a special commit called "code cleanup" that pollutes history and I hate you for that), lines too long, if you used nested ifs or returns, unless it's really nasty code (which I very rarely stumbled upon, usually written by some coding style nazzi in his youth);<p>Feedback for my own/others code:<p>- this or that "coding style" issues by some coding style nazzi; almost fine, it improved my inner compiler/code formatter<p>- "don't use this language feature, because I don't like it" from the highly oppinionated nutjob that leaves the company in a couple of months anyway to join a smaller company where his genius is appreciated more<p>- "why did you do this and that, you should have done...." half an hour later in person/over the phone conversation... "oh, yeah, I guess that actually makes sense"<p>- "whaaaaaat, you expect me review this monster commit? Why didn't you break it in multiple smaller commits?" -- because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now<p>- "How could this security bug have slipped us, we have CODE REVIWS, don't we?"
I am really looking for a "code review is a waste of time, let's skip it" article. Seems like there's a market need that isn't being fulfilled.