4 ms·

I've found code reviews to be a waste of time. Here's my primary concern: what evidence do you have that this code actually works? I want you to show me your te

I've found code reviews to be a waste of time. Here's my primary concern: what evidence do you have that this code actually works? I want you to show me your test plan, I want you to show me you thought through how this code is going to be used and have tests for it. I'm way more interested in that than I am whether variable names use camel case or snake case or whatever. In fact if your tests are complete, and pass, then we can always refactor the code later if we need to.

mathgladiator 6y ago
This all depends on your dev environment; a code review should also have tests with the diff as well as test results, and the more automated the better.
spaetzleesser 6y ago
I often find stuff and also get suggestions for things that could have been done better. I view code reviews more as a mutual learning experience similar to pair programming. Checking for variable names is silly.
- aequitas 6y ago
  When I stopped seeing code reviews as gate keepers and started seeing them as collaboration and communication points it finally made sense to me. But reviews only work well if your team shares a common goal, like maintainability or knowledge transfer. I've worked in a place where code reviews where treated to just a box to have checked off. There was no shared responsibility for the codebase. And most people had a mentality of "if you approve mine, I'll approve yours". Still it was kept as a "safeguard" by management because for them it was also a checkbox they could mark on their process checklist. I'm happy I left, there is only so much culture a single person can change in a team/company.
dougmwne 6y ago
In QA I have found a code review to be very helpful in identifying test cases. Often I'll think I've covered every edge case I can think of, but once I've read through the code I identify the implementation can fail in a certin scenario I hadn't considered. This is why I think Black Box testing is a bad approach unless something about the circumstance specifically calls for it. I do think it's better to have a test plan first, then supplement it with code review, just to not get overly biased.
dm03514 6y ago
I have the same mentality. I feel like almost everything is an implementation detail. Every person's implementation is likely to be different. In my experiences, even the poorest code is maleable (i.e. software :p). PR's become more of a gate to see if you understand the problem and test for the solution opposed to HOW you went about it. Maybe I've had bad experiences but focusing on HOW is really prone to toxicity. I try to avoid that in PRs at all cost and foster a culture of collaboration, shared understanding of the PROBLEM and Respect for the Constraints. I've left places because of extremely toxic PR culture, so I'm probably pretty jaded :p (The culture was so bad I dreaded asking for reviews.) How did you prove that your solution works and respects the constraints?? How do you protect yourself and others from future changes? Everything else is an implementation detail.
jaxwerk 6y ago
> 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.
- taylodl 6y ago
  > 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. 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.
aequitas 6y ago
Bingo. If your function is properly tested or your _intent_ is properly documented/commented I don't care how the code looks (to a degree), as long as I have all the information I need to refactor it right there. Also reviewers shouldn't squabble about syntax or formatting, that what we got computers for to do for us using auto formatting tools. Though it does help if a team converges on a similar style for which reviews are great as you quickly learn each others style and cross contaminate them with your own.

mathgladiator 6y ago

This all depends on your dev environment; a code review should also have tests with the diff as well as test results, and the more automated the better.

spaetzleesser 6y ago

I often find stuff and also get suggestions for things that could have been done better. I view code reviews more as a mutual learning experience similar to pair programming. Checking for variable names is silly.

aequitas 6y ago
When I stopped seeing code reviews as gate keepers and started seeing them as collaboration and communication points it finally made sense to me. But reviews only work well if your team shares a common goal, like maintainability or knowledge transfer. I've worked in a place where code reviews where treated to just a box to have checked off. There was no shared responsibility for the codebase. And most people had a mentality of "if you approve mine, I'll approve yours". Still it was kept as a "safeguard" by management because for them it was also a checkbox they could mark on their process checklist. I'm happy I left, there is only so much culture a single person can change in a team/company.

dougmwne 6y ago

In QA I have found a code review to be very helpful in identifying test cases. Often I'll think I've covered every edge case I can think of, but once I've read through the code I identify the implementation can fail in a certin scenario I hadn't considered. This is why I think Black Box testing is a bad approach unless something about the circumstance specifically calls for it. I do think it's better to have a test plan first, then supplement it with code review, just to not get overly biased.

dm03514 6y ago

I have the same mentality. I feel like almost everything is an implementation detail. Every person's implementation is likely to be different. In my experiences, even the poorest code is maleable (i.e. software :p). PR's become more of a gate to see if you understand the problem and test for the solution opposed to HOW you went about it. Maybe I've had bad experiences but focusing on HOW is really prone to toxicity. I try to avoid that in PRs at all cost and foster a culture of collaboration, shared understanding of the PROBLEM and Respect for the Constraints. I've left places because of extremely toxic PR culture, so I'm probably pretty jaded :p (The culture was so bad I dreaded asking for reviews.) How did you prove that your solution works and respects the constraints?? How do you protect yourself and others from future changes? Everything else is an implementation detail.

jaxwerk 6y ago

> 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.

taylodl 6y ago
> 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. 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.

aequitas 6y ago

Bingo. If your function is properly tested or your _intent_ is properly documented/commented I don't care how the code looks (to a degree), as long as I have all the information I need to refactor it right there. Also reviewers shouldn't squabble about syntax or formatting, that what we got computers for to do for us using auto formatting tools. Though it does help if a team converges on a similar style for which reviews are great as you quickly learn each others style and cross contaminate them with your own.