I'm going to agree in general, and if somebody finds they're lacking a process, this is a decent starting point.
My opinions differ on a few points:
I don't see a reason to always find something to comment on. Perhaps this is a symptom of having larger branches. We quite often have small ones where there is just nothing wrong with. I also disagree on commenting too much on trivial things. You have to consider the morale the submitting programmer; being too picky causes unnecessary stress. Save the comments for important stuff.
"Try to break the code!" is an open-ended and infinite requirement. There is no such thing as 100% coverage. Code can always be broken. There is no value in finding ways to break code that won't be within your supported use-cases. It's better to encourage defensive programming and try to fail gracefully instead of testing in unnecessary features.
I also lean towards trusting submitters more than starting from a position of uncertainty. I don't understand our entire code base. It's too large for all of our team to know every aspect of it. I'll trust that when somebody submits a defect-fix that it is somehow necessary, even if I don't understand it completely, or at all in some cases. New features will have a much higher level of understanding required than bug fixes.
I don't see a reason to checkout, build the code, and test it myself. I rely on the CI system to be doing these basic checks for me. Manually doing this step would take a lot of my time and yield little to no benefit -- unless there is something specific I wish to check.
By the way (and separate from my rebuttal), I really do think you make some very good points that bear consideration - I just edited the post to draw attention to this thread.
I think you make some valid points, and perhaps our processes better fit our organization than your project. At the same time, I would like to point out that "trusting the contributor" is very treacherous water indeed, because we get code blind. By way of example, I am the most senior developer at MousePaw Media, and the most familiar with the code, but I can point to many cases where an intern found a major flaw in my code, that would have been MUCH harder to catch had the code landed and shipped.
I know that reviewer time and effort is not inexhaustible (as Idan pointed out), but neither is the coder's time and effort. These practices are an investment. Far more time is spent trying to catch and fix shipped bugs than is spent catching them in pre-commit review to begin with.
As to the building step, remember that I said to trust the CI. Basic build problems should be caught there, and if it can build, any build problems on your end are basically your own. The reason I say to test is because automatic tests aren't perfect. Perhaps this is because, right now, we're mainly working in library and API design, but I have found in many cases that there is a MASSIVE gap between "passes unit tests" and "works in real life". These problems are only caught if someone actually tries to use the code.
I want to agree with and amend one other thing you pointed out - we can't all understand the entire code base. I certainly don't! But there is a difference between understanding the changes and understanding all the code. One can aim to understand all the changed code, while taking the unchanged code "for granted". Retrospect, I should have made this clearer.
For example, let's imagine the following is the only change in a file:
for(varc=0;c<cities;++c){// Print out the name and current temperature of each city.cout<<cityDB.get(c)->name)<<endl;cout<<cityDB.get(c)->temp)<<endl;c=c+1;}
We might glance at the code for cityDB.get() to be sure it returns a pointer to something with the functions name() and temp(), but for the most part, we can just assume that these things are defined and work correctly. There's no need to fully read and understand this code to see that it is being used correctly.
However, in fully understanding the change, we can spot an error: the third line of the loop block increments the loop iterator, meaning we're skipping every other city! Maybe this was translated from a while loop, or maybe the programmer's brain just ate a SPARC, but we can spot a problem that we'd have missed if we "trusted" the contributor too much.
Furthermore, what if a casual glance at cityDB revealed an actual iterator class built into it? Using that would be far more efficient, and that would also warrant a helpful comment here.
It's a tricky balance in practice. We don't have the time to understand everything. Yet, at our company, one project got indefinitely tabled because only one developer actually understood the code. That's never a good position to get one's organization into.
--
Lastly, yes, I know "find something to comment on" may be slightly overkill, but I hope basic discernment can speak into this. If we can actually say "this code needs no improvement," then we should do so and move on; however, we should be certain our comprehension of the code yields that conclusion, and we're not just jumping to it because we're lazy/tired/whatever. If we can't review it properly, we shouldn't be reviewing it at all.
I understand your concern about the product being useful. It's unfortunately common that programmer's produce things that don't actually work as intended, often because they didn't test it themselves, or there is a high-level compatibility. I have two approaches to get out of that environment:
Don't focus on low-level unit tests. Though they can be useful for debugging, they don't show much of whether something works. I'm totally happy testing low-level bits via their high-level function. I've been meaning to write an article about this a bit more... but the idea is that 100% isolated code coverage in tests is worthless compared to 10% high-level coverage.
I understand the problem you're solving with your approach. I agree you need a solution to the problem.
I guess trust depends on how well you know them. I primarily deal with a team I know. Our 3rd party contributions get a more rigourous review. But I don't mean about small details here, I mainly mean about the purpose of the fix. Certainly, even for code where I don't undrestand the goal I can still check several details of how it works. I can catch obvious failures even if I don't know.
For new vs. old code, yes, by all means assume the old code works. It's unfair to penalize pull requests because the old stuff needs improvement -- I even let some bad style slip through if it mimics the existing style. But there is some code that I just don't expect others to understand.
This is domain specific, and deals a lot with specialty algorithms usually. For example, I recently found a bug in the code I used to measure the length of vector paths. It took me a long time researching and finding the algorithms to begin with. Unless we want a reviewer to do the same research, and better, they simply would not have found the issue. They could understand the method names, and surroudning code, but the core algorithms present a bit of problem when it comes to reviewing.
For the same reason I just to have accept hacky workarounds #1 throuhg #7 on an Android target for our product. I assume the submitter did testing and research. I can verify the code is technically correct, ensure there's a manual test bit, but without spending lots of time I really can't say for sure if it's the correct approach, or even valid.
Idar Arye brings up a good point baout ROI as well. It's an unfortunate reality, that often it's more efficient, as a business, to ship buggy features (refer to Are we forever cursed with buggy software?. This is not an excuse though. The decision to trade priorites shouldn't be haphazard or done without thought.
Again, and this bears repeating: I agree code review should have rules and goals. I'm arguing only about some of the fine details here. Nobody should read this and come to the conclusion that the process is wrong. There are some details where I have alternate solutions, or have [hopefully] well reasoned objections.
Quality assurance is either a constant battle or it's being done wrong. :)
I don't see a reason to checkout, build the code, and test it myself. I rely on the CI system to be doing these basic checks for me. Manually doing this step would take a lot of my time and yield little to no benefit -- unless there is something specific I wish to check.
+1 This is exactly what automated testing is such a powerful tool.
I concur. These practices may help catching problems, but they seem to have a very low RoI. The reviewrs' time and effort are not inexhaustible resources.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
I'm going to agree in general, and if somebody finds they're lacking a process, this is a decent starting point.
My opinions differ on a few points:
By the way (and separate from my rebuttal), I really do think you make some very good points that bear consideration - I just edited the post to draw attention to this thread.
I think you make some valid points, and perhaps our processes better fit our organization than your project. At the same time, I would like to point out that "trusting the contributor" is very treacherous water indeed, because we get code blind. By way of example, I am the most senior developer at MousePaw Media, and the most familiar with the code, but I can point to many cases where an intern found a major flaw in my code, that would have been MUCH harder to catch had the code landed and shipped.
I know that reviewer time and effort is not inexhaustible (as Idan pointed out), but neither is the coder's time and effort. These practices are an investment. Far more time is spent trying to catch and fix shipped bugs than is spent catching them in pre-commit review to begin with.
As to the building step, remember that I said to trust the CI. Basic build problems should be caught there, and if it can build, any build problems on your end are basically your own. The reason I say to test is because automatic tests aren't perfect. Perhaps this is because, right now, we're mainly working in library and API design, but I have found in many cases that there is a MASSIVE gap between "passes unit tests" and "works in real life". These problems are only caught if someone actually tries to use the code.
I want to agree with and amend one other thing you pointed out - we can't all understand the entire code base. I certainly don't! But there is a difference between understanding the changes and understanding all the code. One can aim to understand all the changed code, while taking the unchanged code "for granted". Retrospect, I should have made this clearer.
For example, let's imagine the following is the only change in a file:
We might glance at the code for
cityDB.get()to be sure it returns a pointer to something with the functionsname()andtemp(), but for the most part, we can just assume that these things are defined and work correctly. There's no need to fully read and understand this code to see that it is being used correctly.However, in fully understanding the change, we can spot an error: the third line of the loop block increments the loop iterator, meaning we're skipping every other city! Maybe this was translated from a
whileloop, or maybe the programmer's brain just ate a SPARC, but we can spot a problem that we'd have missed if we "trusted" the contributor too much.Furthermore, what if a casual glance at
cityDBrevealed an actual iterator class built into it? Using that would be far more efficient, and that would also warrant a helpful comment here.It's a tricky balance in practice. We don't have the time to understand everything. Yet, at our company, one project got indefinitely tabled because only one developer actually understood the code. That's never a good position to get one's organization into.
--
Lastly, yes, I know "find something to comment on" may be slightly overkill, but I hope basic discernment can speak into this. If we can actually say "this code needs no improvement," then we should do so and move on; however, we should be certain our comprehension of the code yields that conclusion, and we're not just jumping to it because we're lazy/tired/whatever. If we can't review it properly, we shouldn't be reviewing it at all.
I understand your concern about the product being useful. It's unfortunately common that programmer's produce things that don't actually work as intended, often because they didn't test it themselves, or there is a high-level compatibility. I have two approaches to get out of that environment:
I understand the problem you're solving with your approach. I agree you need a solution to the problem.
I guess trust depends on how well you know them. I primarily deal with a team I know. Our 3rd party contributions get a more rigourous review. But I don't mean about small details here, I mainly mean about the purpose of the fix. Certainly, even for code where I don't undrestand the goal I can still check several details of how it works. I can catch obvious failures even if I don't know.
For new vs. old code, yes, by all means assume the old code works. It's unfair to penalize pull requests because the old stuff needs improvement -- I even let some bad style slip through if it mimics the existing style. But there is some code that I just don't expect others to understand.
This is domain specific, and deals a lot with specialty algorithms usually. For example, I recently found a bug in the code I used to measure the length of vector paths. It took me a long time researching and finding the algorithms to begin with. Unless we want a reviewer to do the same research, and better, they simply would not have found the issue. They could understand the method names, and surroudning code, but the core algorithms present a bit of problem when it comes to reviewing.
For the same reason I just to have accept hacky workarounds #1 throuhg #7 on an Android target for our product. I assume the submitter did testing and research. I can verify the code is technically correct, ensure there's a manual test bit, but without spending lots of time I really can't say for sure if it's the correct approach, or even valid.
Idar Arye brings up a good point baout ROI as well. It's an unfortunate reality, that often it's more efficient, as a business, to ship buggy features (refer to Are we forever cursed with buggy software?. This is not an excuse though. The decision to trade priorites shouldn't be haphazard or done without thought.
Again, and this bears repeating: I agree code review should have rules and goals. I'm arguing only about some of the fine details here. Nobody should read this and come to the conclusion that the process is wrong. There are some details where I have alternate solutions, or have [hopefully] well reasoned objections.
Quality assurance is either a constant battle or it's being done wrong. :)
+1 This is exactly what automated testing is such a powerful tool.
I concur. These practices may help catching problems, but they seem to have a very low RoI. The reviewrs' time and effort are not inexhaustible resources.