27 ms·

It's the same in the financial sector, you can't just push code without a review. However, the review process is far from perfect and can create a false sense

It's the same in the financial sector, you can't just push code without a review. However, the review process is far from perfect and can create a false sense of security. To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job. If there were better incentives for doing a review, then it would greatly improve the situation. But i haven't seen that yet.

XorNot 5y ago
I've had a lot of trouble getting people to do proper reviews. A minimum to me is that you actually compile and execute the code in some way to check its sane. Better would be the reviewer actually adds to the test suite for the code to prove their expectations of how it works. In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole context. The other problem I've found is stamping down on style arguments: they have no place in code reviews, and a lot of the time you get a round of "please change this variable to be named something else" - which might be valid, but raises the question of why the reviewer didn't do it, and more often leaves a weird authority gap - I have no power to just reject such a request because I think it's invalid, nor does the business have anyway to resolve the issue. In fact if there's a dispute at all, very rarely is there any process for bringing in a third party to mediate or break the stalemate (which is hilarious in some ways since everyone by now should realize 2-party systems can't achieve consensus). Code reviews are very, very cargo-culty and I've not seen them ever include a thorough design: people give up on the very near edge cases, and so the whole system falls apart because it's just a hurdle to get through and not a collaborative or constructive process.
- kqr 5y ago
  Why should it be on the reviewer to compile and execute the code, rather than the author? Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one? When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.
  - DoctorDabadedoo 5y ago
    Not OP here. Code reviews, to me, are opportunities to 1) Catch edge use case / business requirements misunderstandings 2) Mentor new hires/junior engineers on the ways things should be done (e.g. internal culture about some architecture/design). BUT, I absolutely despise code reviews that nitpick on non documented or 'because I prefer that way' stuff: 1) Task tickets that only have a vague description, have their own 'play detective' about the requirements, but a lot of new ones surface during code review. 2) Requirements are met, but there are internal/personal technical directives that are written nowhere, are not enforced with automated checks and may dramatically change a feature/patch is implemented (I can only think about that guy that worked in a company in which only LEFT JOIN sql statements were allowed). Fuck that noise.
- ahtihn 5y ago
  Compiling and running the code is handled by a build server and automated tests. That's not the point of code reviews. The main purpose of code reviews is checking the general structure of the code and tests. Is the code well designed? Are error cases considered and tested? Another important purpose of code reviews is knowledge sharing.

XorNot 5y ago

I've had a lot of trouble getting people to do proper reviews. A minimum to me is that you actually compile and execute the code in some way to check its sane. Better would be the reviewer actually adds to the test suite for the code to prove their expectations of how it works. In almost all cases it's very hard to get people to look outside the web browser for the diffs. Diffs show you something, but never the whole context. The other problem I've found is stamping down on style arguments: they have no place in code reviews, and a lot of the time you get a round of "please change this variable to be named something else" - which might be valid, but raises the question of why the reviewer didn't do it, and more often leaves a weird authority gap - I have no power to just reject such a request because I think it's invalid, nor does the business have anyway to resolve the issue. In fact if there's a dispute at all, very rarely is there any process for bringing in a third party to mediate or break the stalemate (which is hilarious in some ways since everyone by now should realize 2-party systems can't achieve consensus). Code reviews are very, very cargo-culty and I've not seen them ever include a thorough design: people give up on the very near edge cases, and so the whole system falls apart because it's just a hurdle to get through and not a collaborative or constructive process.

kqr 5y ago
Why should it be on the reviewer to compile and execute the code, rather than the author? Or are you saying that one can actually find more bugs when two people compile and execute the same code rather than just one? When I review code, I want the author to tell me how they have verified that the code has the desired effect -- but if they do that, I'm going to trust that they did the actual verification and I'm not going to simply retrace their steps. That seems like a waste of time to me.
- DoctorDabadedoo 5y ago
  Not OP here. Code reviews, to me, are opportunities to 1) Catch edge use case / business requirements misunderstandings 2) Mentor new hires/junior engineers on the ways things should be done (e.g. internal culture about some architecture/design). BUT, I absolutely despise code reviews that nitpick on non documented or 'because I prefer that way' stuff: 1) Task tickets that only have a vague description, have their own 'play detective' about the requirements, but a lot of new ones surface during code review. 2) Requirements are met, but there are internal/personal technical directives that are written nowhere, are not enforced with automated checks and may dramatically change a feature/patch is implemented (I can only think about that guy that worked in a company in which only LEFT JOIN sql statements were allowed). Fuck that noise.
ahtihn 5y ago
Compiling and running the code is handled by a build server and automated tests. That's not the point of code reviews. The main purpose of code reviews is checking the general structure of the code and tests. Is the code well designed? Are error cases considered and tested? Another important purpose of code reviews is knowledge sharing.