8 ms·

On Code Review

75 points by HugoDias 5y ago

deleted 5y ago
[deleted]
chrismorgan 5y ago
I also find code review to be an exceptionally 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 the 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, &c. &c.
posharma 5y ago
Hasn't enough been already said and written about the importance and effectiveness of code reviews? Do we really need yet another article on it?
- mskec 5y ago
  Not everyone reads all the articles. This one has been very useful to me and I'm thankful for the submission.
  - posharma 5y ago
    Any software engineering book worth its salt covers this topic. Besides a quick internet search on code review best practices will give you several well written articles. What’s the point of writing and discussing the same thing repeatedly. Seriously move on already! My point is why beat a dead horse. Instead write about something new.
V-2 5y ago
"Approving a pull request without even testing the code it’s very dangerous" 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. 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. 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. "some 2~3 line changes may not need to be tested, but those are the exception, not the rule." Generally speaking everything needs to be tested (not necessarily manually), but that's beyond the point. Testing is not reviewing, and vice versa.
- fredrb 5y ago
  > 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. I'm 100% on-board with this. However, I understand were OP's mindset comes from. A few years ago I was working at a development shop in a fairly large project, where we had little to no automated tests. We frequently had PRs that would break main features. At some point developers were required to perform smoke tests alongside code reviews.
  - deleted 5y ago
    [deleted]
- kashug 5y ago
  I agree that code review is not QA. But I read it as the writer of the post comes from a team where they do QA on pull-requests. And QA is assumed to be done on a per PR basis. But here I think different teams will have different approaches and pipelines. On my current team, we want most tests to be automatic. But the manual regression testing that might be needed is mostly done by other developers. A separate test-environment is created for each PR, and the link posted in the pull-request. So when looking at a pull-quest we expect people to do both a code-review and QA. In my team approving a PR means you approve for the code to go to production. So you need to consider both code quality and whatever QA is needed. But thats just our way t do it - other teams do things differently. And I think thats OK, there is no silver bullet.
codeapprove 5y ago
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) I'm building CodeApprove (https://codeapprove.com https://codeapprove.com) 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.
habitue 5y ago
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.
- V-2 5y ago
  Sounds like the good, old "X considered harmful" template, where the headline is clickbaity, but the content actually boils down to "X considered harmful ...if done badly" (doh).
gocartStatue 5y ago
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. 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.
- V-2 5y ago
  "I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent." This implies the need for smaller pull requests maybe? If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on. Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late. "If you want to enforce standards, automate it" Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there! However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like. "If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor" Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years? "The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.
  - KronisLV 5y ago
    > This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner. Otherwise you end up with something like the following being separate requests: - FEATURE-123 add data models for the feature - FEATURE-123 add service logic for the feature - FEATURE-123 add logging and validations for the feature - FEATURE-123 add unit tests for the feature - FEATURE-123 add REST endpoints for the feature - FEATURE-123 add front end code to forms - FEATURE-123 add front end logic to use the REST endpoints - FEATURE-123 add integration tests for headless browser You don't actually want these changes to end up in the main branch before they are done. Alone, they provide no actual value and could only confuse people. Furthermore, if there are changes that are needed to the data model after it has already been merged, then you'd also need to do those within a later merge request, or one in the middle, which would just pollute the history and serve as spam, instead of being able to add more commits to the other sub request. To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives. EDIT: So, you'd need something like the following: - (WIP, 3/8 reviewed) FEATURE-123 add new feature // feature-123 branch, cannot be merged into main yet, review the below first - (reviewed) add data models for the feature // feature-123-data-models branch, will be merged into previous - (reviewed) add service logic for the feature // feature-123-service-logic branch, will be merged into previous - (reviewed) add logging and validations for the feature // feature-123-logging-and-validations branch, will be merged into previous - (in review, 9/13 comments resolved) add unit tests // feature-123-unit-tests branch, will be merged into previous - (registered) add REST endpoints for the feature // feature-123-rest-endpoints branch, will be merged into previous - (registered) add front end code to forms // feature-123-front-end-code branch, will be merged into previous - (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous - (registered) add integration tests for headless browser // feature-123-integration-tests branch, will be merged into previous (personally i think merges make the most sense, but some people prefer rebasing; it's just an example) You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.
raducu 5y ago
I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. 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); Feedback for my own/others code: - this or that "coding style" issues by some coding style nazzi; almost fine, it improved my inner compiler/code formatter - "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 - "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" - "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 - "How could this security bug have slipped us, we have CODE REVIWS, don't we?"
- eertami 5y ago
  > 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 Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.
  - pm215 5y ago
    Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...
andrewingram 5y ago
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.
- redis_mlc 5y ago
  > can actually be a net negative Gen Z gets perpetually triggered, so there is a move to not do code reviews. I just make an effort to be first to review and keep the mood light.
- freddref 5y ago
  Do you have links to reading on the arguments against code review?
  - andrewingram 5y ago
    I'll see what I can dig up. Some of it is from Allen Holub, who I appreciate is often met with mixed responses here; but his angle is mostly that if you have sufficient up-front collaboration (he favours mobbing), code review don't offer you anything on top of what automated tools provide, and can actually erode team trust/morale because they're perceived as a barrier often introduced as a way to say "no" to people. I don't agree 100%, but I am on-board with the sentiment. I've seen systemic problems of under-communication pre-review which results in me looking at a PR and seeing a fundamental issue, but not knowing how to approach tackling it without hurting feelings. I don't think eliminating reviews is the solution, but I think it's much less of a critical step than it's often made out to be. My take is that if reviews are actually preventing major issues with released code, they're almost certainly including work that should've happened much earlier in the development process.
- deleted 5y ago
  [deleted]
- ozim 5y ago
monster_group 5y ago
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.
- CraigJPerry 5y ago
  In those environments i'd expect a tech lead to take the initiative to stand up and explain / defend the team's quality control processes. If the management rebukes, you move employer. Quality is not something the customer or your manager is responsible for. It's entirely on us as professional developers. That said - code review is not the only way to ensure quality. Do what works. Pairing generally works even better for getting quality review time in.
- detaro 5y ago
  These things obviously are to a degree a matter of team-level process and policy. In other teams it might very well be "good luck getting that promotion if you constantly refuse to participate properly in code reviews".
pfdietz 5y ago
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.

75 points by HugoDias 5y ago

deleted 5y ago

[deleted]

I also find code review to be an exceptionally 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 the 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, &c. &c.

posharma 5y ago

Hasn't enough been already said and written about the importance and effectiveness of code reviews? Do we really need yet another article on it?

mskec 5y ago
Not everyone reads all the articles. This one has been very useful to me and I'm thankful for the submission.
- posharma 5y ago
  Any software engineering book worth its salt covers this topic. Besides a quick internet search on code review best practices will give you several well written articles. What’s the point of writing and discussing the same thing repeatedly. Seriously move on already! My point is why beat a dead horse. Instead write about something new.

V-2 5y ago

"Approving a pull request without even testing the code it’s very dangerous" 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. 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. 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. "some 2~3 line changes may not need to be tested, but those are the exception, not the rule." Generally speaking everything needs to be tested (not necessarily manually), but that's beyond the point. Testing is not reviewing, and vice versa.

fredrb 5y ago
> 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. I'm 100% on-board with this. However, I understand were OP's mindset comes from. A few years ago I was working at a development shop in a fairly large project, where we had little to no automated tests. We frequently had PRs that would break main features. At some point developers were required to perform smoke tests alongside code reviews.
- deleted 5y ago
  [deleted]
kashug 5y ago
I agree that code review is not QA. But I read it as the writer of the post comes from a team where they do QA on pull-requests. And QA is assumed to be done on a per PR basis. But here I think different teams will have different approaches and pipelines. On my current team, we want most tests to be automatic. But the manual regression testing that might be needed is mostly done by other developers. A separate test-environment is created for each PR, and the link posted in the pull-request. So when looking at a pull-quest we expect people to do both a code-review and QA. In my team approving a PR means you approve for the code to go to production. So you need to consider both code quality and whatever QA is needed. But thats just our way t do it - other teams do things differently. And I think thats OK, there is no silver bullet.

codeapprove 5y ago

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) I'm building CodeApprove (https://codeapprove.com https://codeapprove.com) 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.

habitue 5y ago

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.

V-2 5y ago
Sounds like the good, old "X considered harmful" template, where the headline is clickbaity, but the content actually boils down to "X considered harmful ...if done badly" (doh).

gocartStatue 5y ago

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

V-2 5y ago
"I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent." This implies the need for smaller pull requests maybe? If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on. Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late. "If you want to enforce standards, automate it" Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there! However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like. "If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor" Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years? "The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.
- KronisLV 5y ago
  > This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner. Otherwise you end up with something like the following being separate requests: - FEATURE-123 add data models for the feature - FEATURE-123 add service logic for the feature - FEATURE-123 add logging and validations for the feature - FEATURE-123 add unit tests for the feature - FEATURE-123 add REST endpoints for the feature - FEATURE-123 add front end code to forms - FEATURE-123 add front end logic to use the REST endpoints - FEATURE-123 add integration tests for headless browser You don't actually want these changes to end up in the main branch before they are done. Alone, they provide no actual value and could only confuse people. Furthermore, if there are changes that are needed to the data model after it has already been merged, then you'd also need to do those within a later merge request, or one in the middle, which would just pollute the history and serve as spam, instead of being able to add more commits to the other sub request. To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives. EDIT: So, you'd need something like the following: - (WIP, 3/8 reviewed) FEATURE-123 add new feature // feature-123 branch, cannot be merged into main yet, review the below first - (reviewed) add data models for the feature // feature-123-data-models branch, will be merged into previous - (reviewed) add service logic for the feature // feature-123-service-logic branch, will be merged into previous - (reviewed) add logging and validations for the feature // feature-123-logging-and-validations branch, will be merged into previous - (in review, 9/13 comments resolved) add unit tests // feature-123-unit-tests branch, will be merged into previous - (registered) add REST endpoints for the feature // feature-123-rest-endpoints branch, will be merged into previous - (registered) add front end code to forms // feature-123-front-end-code branch, will be merged into previous - (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous - (registered) add integration tests for headless browser // feature-123-integration-tests branch, will be merged into previous (personally i think merges make the most sense, but some people prefer rebasing; it's just an example) You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.

raducu 5y ago

I must be the exception, but I've only had bad or really "meh" experiences with code reviews: Doing the reviews myself: A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs. if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case There's nothing to review, really, I don't know the business case, the code looks okish. 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); Feedback for my own/others code: - this or that "coding style" issues by some coding style nazzi; almost fine, it improved my inner compiler/code formatter - "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 - "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" - "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 - "How could this security bug have slipped us, we have CODE REVIWS, don't we?"

eertami 5y ago
> 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 Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.
- pm215 5y ago
  Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...

andrewingram 5y ago

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.

redis_mlc 5y ago
> can actually be a net negative Gen Z gets perpetually triggered, so there is a move to not do code reviews. I just make an effort to be first to review and keep the mood light.
freddref 5y ago
Do you have links to reading on the arguments against code review?
- andrewingram 5y ago
  I'll see what I can dig up. Some of it is from Allen Holub, who I appreciate is often met with mixed responses here; but his angle is mostly that if you have sufficient up-front collaboration (he favours mobbing), code review don't offer you anything on top of what automated tools provide, and can actually erode team trust/morale because they're perceived as a barrier often introduced as a way to say "no" to people. I don't agree 100%, but I am on-board with the sentiment. I've seen systemic problems of under-communication pre-review which results in me looking at a PR and seeing a fundamental issue, but not knowing how to approach tackling it without hurting feelings. I don't think eliminating reviews is the solution, but I think it's much less of a critical step than it's often made out to be. My take is that if reviews are actually preventing major issues with released code, they're almost certainly including work that should've happened much earlier in the development process.
deleted 5y ago
[deleted]
ozim 5y ago

monster_group 5y ago

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.

CraigJPerry 5y ago
In those environments i'd expect a tech lead to take the initiative to stand up and explain / defend the team's quality control processes. If the management rebukes, you move employer. Quality is not something the customer or your manager is responsible for. It's entirely on us as professional developers. That said - code review is not the only way to ensure quality. Do what works. Pairing generally works even better for getting quality review time in.
detaro 5y ago
These things obviously are to a degree a matter of team-level process and policy. In other teams it might very well be "good luck getting that promotion if you constantly refuse to participate properly in code reviews".

pfdietz 5y ago

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.