| > Here's my primary concern: what evidence do you have that this code actually works? This is a strange concern to try to address with code-review. I don't think most folks actually expect to catch a lot of bugs by staring at diffs. Testing is the right tool for that job. > I'm way more interested in that than I am whether variable names use camel case or snake case or whatever. Style concerns definitely shouldn't be the primary focus of a code review. Ensuring some stylistic consistency through the code base is a benefit they can provide, but certainly not the the most major. > In fact if your tests are complete How do you know if the tests are complete on someone's work before they merge it? Don't you need to get in there, and well, review their code to see what tests they wrote? Evaluation of test coverage by a peer is one of the more important parts of a good code review. What other process would you use to ensure incoming patches have the test coverage you want them to have? > we can always refactor the code later if we need to. Isn't it better to catch code structure issues when they are fresh in everyone's mind rather than taking on technical debt and addressing it down the road when it will require more effort? We've all written code in somewhat silly ways. My experience is that when a peer calls that out before I merge it, it's way easier to fix up-front than a year down the road when I'm in there trying to fix some bug and am wondering if I should just refactor the mess while I'm at it. |
Correct. I expect those tests to be developed alongside the code. Part of my 'definition of done' is you have passing tests to prove the implementation is complete.
> Style concerns definitely shouldn't be the primary focus of a code review. Ensuring some stylistic consistency through the code base is a benefit they can provide, but certainly not the the most major.
Agreed. However, I've seen too many toxic scenarios where this is what actually happens.
> How do you know if the tests are complete on someone's work before they merge it?
Everything at the branch point can be tested. Also, since we have tests, we have the ability to regression test and ensure everything is still working properly after the merge.
> Isn't it better to catch code structure issues when they are fresh in everyone's mind rather than taking on technical debt and addressing it down the road when it will require more effort?
Yes. My counterargument though is having a complete set of tests that all pass is far more important than concerning yourself with the structure of the code. I understand that's a radical departure from the state of the art but it stems from years and years and years, nay decades, of enduring counterproductive code reviews. I've sat in on several sessions where there have been long, drawn-out discussions on flexibility and extensibility when 9 times out of 10 it simply didn't matter. I've seen so much time wasted on stuff that ultimately didn't matter.
My take away: make sure your tests are reasonably complete and they pass. If so then you're good. Maybe take a quick look at the code, especially if being submitted by a junior, to make sure nothing outrageous is present, otherwise don't worry too much about it.