9 ms·

Code reviews need to shift-left

37 points by rekahrv 4y ago

rekahrv 4y ago
An intriguing article, although my opinion on async reviews is much more positive. :-) There are many arguments for both sides. Pro pair / mob programming: * Faster feedback. * Less time spent on solutions that might be rejected. * Better knowledge sharing (for those who participate). Pro async review: * A more neutral, "external" view. (Smaller chance of groupthink.) * More empathy with future readers of the code. * Encourages to make assumptions and decisions explicit. => Better knowledge sharing for those, who weren't involved in the writing of the code.
ampersandy 4y ago
I don't really buy the two main criticisms around code review. They're stated as intractable downsides but are really just cultural issues around how you perform code reviews. If your coworkers are sending changes for review that don't explain why the changes are being made (and therefore provide the context), reject it until they write a proper summary/test plan that does explain it. You should review the "metadata" of changes before you ever review the code itself and if done well, you'll probably be able to guess what a lot of the incoming changes are before reviewing the code. This makes it easier to spot code that deviates from the stated purpose of the changes and can help identify faults in both understanding of the overall problem or the system being built, especially in more junior engineers. If your coworkers are taking to long to review changes, work with your team to incentivize expedient code reviews, or talk to the people that aren't reviewing enough code to help everyone else get more done. You should also utilize a stack that lets you continue progressing with your work even when waiting for reviews (like stacked diffs).
- watwut 4y ago
  The context is not just "why the change was made". That point is usually clear from ticket in the first place. The context is "do other places in code use the same tactic" question. It is also the "I tried these four things and three of them failed because of X" or "I had to choose between these two solutions and picked this one".
  - ajorgensen 4y ago
    Having the “why” in the same place as the code I think is critically important. Who knows what ticketing system you’ll use in the future, it’s fine to link to but it shouldn’t be where all the context is stored. If you think about a pull request as a story you have who, what, where, when, and why. Most tools handle the first 4 but the why is critically important to anyone, including yourself, who needs to understand the nuance of the change that can’t easily be expressed by the code itself without a novel size comment block. I find that a lot of times people forget the “why” when submitting a change set and simply restate the “what”. At the time you make the change it’s all fresh in your mind and you trick yourself into believing you’ll remember all the details but in reality that’s no my experience.
  - onion2k 4y ago
    Do those things matter once the code is actually written though? They're important during the dev process, particularly if you're deciding between two different-but-valid approaches, which is why teams should be communicating and discussing things, but once the decision has been made it ceases to be anything worth worrying about. It's a sunk cost. It shouldn't need to be a part of a code review unless the comms are failing ahead of the review process. If that's the case then you should try to fix the comms issue.
    - taeric 4y ago
      A lot of what we devs think are vitally important... just aren't. Or, they have a steep time decay on how useful they are. I do have the longest code review summaries on my team, amusingly. So, I clearly like the idea of it being a discussion and capturing some of that. I would be challenged to say what part is most important overall, though. In large, because not all code changes are the same.
    - rekahrv 4y ago
      "Do those things matter once the code is actually written though?" That's a great question. I guess it depends on how much the code will change in the future. These things CAN get relevant again when you consider changing that code. E.g. if it turns out later that the library I've chosen isn't maintained anymore. Or we would need some more functionality, but this library doesn't provide it. In these situations, it's useful to see which other alternatives we've considered. That said: I can't really estimate how often this turns out to be relevant, indeed. I guess the answer might be very different for different projects.
- rekahrv 4y ago
  'You should review the "metadata" of changes before you ever review the code itself' That's a great approach. It sounds logical, but I certainly don't do it consistently. :-)
- plorkyeran 4y ago
  The first criticism ("Context Gaps. The reviewer doesn’t have the full context of how and why decisions were made for the code they were reviewing.") is one of the things I consider a _advantage_ of code reviews. The article just dismisses the idea that having the reasons for decisions written down is useful, which I find extremely strange.
javaunsafe2019 4y ago
We don’t do code reviews anymore. We have a good quality gate (e2e tests) and pair programming. All development happens on main and gets directly deployed to prod when the tests succeed … I hope I never have to use git flow or alike again in the future!
- jagged-chisel 4y ago
  What's the size of your team? Remote or on-site? How many timezones does it span? How senior are the team's members?
  - sodapopcan 4y ago
    Not the OP but I've worked very successfully pair-programming full-time on a fully remote team of 6 devs, 1 tester, 1 designer, and 1 product manager. All devs were quite senior except for one junior (and that junior leveled up faaaaaasssssst). We usually rotated daily so if a story took three days, the two people who started it weren't the same two who finished it. We all developed a similar coding style for the particular project, everyone knew every part of the codebase to some degree, and we felt a collective ownership over it--there was none of that "Oh, I don't want to change this because Sam wrote it and they'll be mad if I change it" nonsense. However, our timezones only spanned three hours.
  - jross225 4y ago
    I'm on a data team of 10, and after about 6 months of onboarding we have permissions to push directly to main pending passing tests. We generally do this for small changes, or changes where we are the context owner. With the caveat that reviews happen after the code is deployed, and usually within a few days. Personally, I like the process. It allows us to move quickly, and focus on blocking changes. We can still get reviews prior to pushing code if it is sensible (for large changes), but most (80%?) changes tend to be quite small.
    - rekahrv 4y ago
      That's an intriguing approach. It makes sense that not all changes are equal and they don't require the same review process. (And it certainly makes sense to encourage small changes.) What qualifies as a "small change"? Do you have some numbers to measure it? Or is it the developer's call?
bsenftner 4y ago
This reads far too basic, like someone read some other blogs and tried to write one without out blatant plagiarism. It's just code advice cliches.
SCdF 4y ago
This article has a "The Issues with Async Reviews" section but not a "The Issues With Pair Programming and Mobbing" one, so it's not a particularly considered piece. I have found that pair programming destroys the ability to coherently think about a large portion of work for a large portion of developers. It is not easy to be "on" all the time, and a lot of people work at a pace or in a structure that is not compatible with that.
- sodapopcan 4y ago
  I haven't personally found this to be the case at all but pair programming doesn't necessarily mean always working with someone _at all times_. Since I'm a big pairing advocate and I think it's important to pair off every day, but there's absolutely nothing wrong with deciding with your pair that a particular problem needs some alone time.
  - deleted 4y ago
    [deleted]
alkonaut 4y ago
The issue with code reviews is that most of them should be prototype reviews before you are even half way done, or design reviews before you even start. Or just dialog around some code or problem, along the way. Looking for quality issues or obvious mistakes after the fact is very rarely useful. I still think it's a good idea to have them as a sign off (e.g. so you at least have 2 people knowing what's happened), but other than that I don't think they provide that much value, at least not for people who are many years into a project and code base. But still we do these after-the-fact code reviews because I don't want to bother my colleague N times in the process. So I churn away until I'm done. And then not to bother them twice, I also polish it. And THEN I ask for a review. The reviewer sees something polished and finished, and assumes that a) since it's polished I must have spent a lot of thought around whether it's the right thing, and obviously anything they can do in the review time which is never going to exceed 2% of the dev time, is rarely going to result in them second guessing that and b) since it's polished they know they'll probably step on some toes if they say "throw it out and do something completely different". They can see the effort. The relationship with a colleague is more important than even the health of the project. So they approve it. The problem of course then is: how do you get from the "too late code reviews that they are rubber stamps" into a process of dialog, half-way reviews and iteration? Other than pair/mob programming I don't know. We say we don't want more processes, but whenever there isn't a process for this iterative behavior, the result is people developing for too long on their own and producing results that are too far gone to be shot down.
- lexapro 4y ago
  >review time which is never going to exceed 2% of the dev time What, why?
  - alkonaut 4y ago
    Because it’s seen as pure overhead, so is expected to take perhaps a few minutes for a large change (that’s my experience at least). The fact that it isn’t is of course not lost on anyone - but everyone accepts the status quo as preferable. That is: people don’t want to spend a lot of time doing code review because they don’t like it. So they rather spend time working on a worse code base.
tomasreimers 4y ago
(I am biased: I am one of the founders of https://graphite.dev https://graphite.dev ) Flip side of this, you can tackle both of their issues with better tooling and not throw the baby out with the bathwater. Time lag: Stacking (https://graphite.dev/stacking https://graphite.dev/stacking) is a powerful technique that allows developers to keep developing even while others read through their code. Context gap: you encounter the same issue in an IDE when exploring a new part of a codebase, and there are tools to help you there (click to see function def, open relevant tests, AI summarize what its doing - we've been trialing the last one internally and it's so cool). Your CR tool should offer all of that, and at many large companies (Google, FB, MSFT, even Jane street) it does. GitHub is just deficient here. I agree with the other comments in this thread that the main problem is that what should have been an architecture / prototype / etc review happens in CR, and I agree with that. But I think moving away from CR worsens that problem not improves on it.
- vageli 4y ago
  I was intrigued by your post but unfortunately after visiting the site I see no indication of pricing. Since an account is required to submit a stack, I did not proceed without a clear idea of potential cost.
- rekahrv 4y ago
  "Context gap: you encounter the same issue in an IDE when exploring a new part of a codebase" Yes, that's one of my major arguments for async reviews, especially in large organizations: Future readers of the code will have ca. the same context as the async reviewer. (Contrary to a pair programmer, who has much more context.) "there are tools to help you there" That sounds very interesting. Can you elaborate on that? Which tools do you use when getting familiar with a new part of a codebase? Function def and relevant tests are very useful indeed. AI summary what it's doing. That sounds intriguing. Which tools do you use for that? How do you get the broader context? E.g. how do you decide which other parts of the codebase are closely related (and should be included in your exploration)?
throwawaaarrgh 4y ago
What's wrong with leaving the last hour of the day for team code reviews?
drewcoo 4y ago
Am I the only one who thinks it would be incredibly rude to ask for a code review before I'd actually fleshed out my ideas and also cleaned up to make it presentable? For that matter, if early code review is for design decisions, doesn't that mean tasks are large enough to require plans and reviews of those? Shifting code reviews left seems to be either (or both of): - look, I made a poopie! - let's do Big Planning Up Front (agile has failed)
watwut 4y ago
My biggest issue with code review is that people rely on it and trust it too much. It does certain things in the "better then nothing I guess" way, but gets hyped as if it was all there is to teamwork. Yes, it does little bit to spread knowledge - but it is not much effective at that. Actual occasional architecture session where ideas are explained is much better. Yes, it does little bit to catch bugs, but having actual testing in place does much more. Yes, it does something to teach junior, but for Christ sake, currently it leads to "give them task with no guidance, let them figure it out and then tell them about everything they have done wrong". That is just about the worst way of teaching people. And it creates social issues we don't want to acknowledge, because code review is sacred and cant be criticized. Which leads to kindergarten level of advice to people who run into issues like "dont take it personaly, be nice". Which is next to useless if you have an actual social issue going on in your team.
- rekahrv 4y ago
  "My biggest issue with code review is that people rely on it and trust it too much." That's a very good way to put it. Code review is useful, but the expectation around it is often over reasonable. I agree on your points regarding quality and knowledge sharing: Code review is beneficial for both, but definitely not sufficient as the only or main tool. "give them task with no guidance, let them figure it out and then tell them about everything they have done wrong" It sounds blatant. But often not far from what's happening. :-) "And it creates social issues we don't want to acknowledge" Yes, that's another longer topic. :-)
larve 4y ago
I think most issues brought up here would be addressed by writing rfcs and have those go through a review cycle, potentially with attached code that is clearly understood to be a proof of concept. It leverages async by giving someone the autonomy to do clear thinking and clear coding, while avoiding pushing the feedback cycle to when all the work has been done and everybody is stuck with a “lgtm can you reindent the last line” review of a fait accompli . Added benefits are proper top level documentation, and a log of the discussions and compromises made to cut short on future design discussions. Edit: typos on mobile
- corytheboyd 4y ago
  Agreed, the happy path is that code implements a spec that has already been agreed upon, anything less means guess work for both the authors and reviewers of code. Life is about compromise so this doesn't have to be true 100% of the time, but this should be in your team's list of core competencies. Also RFC doesn't HAVE to mean big pedantically formatted and worded monospace document. Templates help make sure common mistakes aren't repeated, but using normal-person English and easy to understand code examples can be very effective.
corytheboyd 4y ago
This is the code review model I've arrived at after going through a number of different processes throughout my career: Code review is a health check from your teammates who don't have the authorship bias. It is not a replacement for testing, both manual and automated. Testing is solely the responsibility of the author, code review is just a design lint check and/or knowledge sharing opportunity. You can still have a QA team of course, but that is usually decoupled from application development and is done post-merge anyway. Every shop is different so YMMV, this is just a personal take.
onion2k 4y ago
Decide if the right structure was chosen: Even if the code is doing the right thing, its structure may be ineffective or inefficient. I can't help thinking that if you're waiting until code review to determine if someone on the team wrote the code in a way that will pass a review or not then you have some serious comms issues on your team. If this is a problem then you should be doing mini reviews during the dev process as well as a code review at the end.
surement 4y ago
pair programming is not a replacement for code reviews; the two people who worked on the code will have the same blind spots (even if the quality of the code is likely going to be better) and a fresh pair of eyes will offer the same benefits as in the regular case
- rekahrv 4y ago
  Yeah, I agree with that. A pair often produces better code than an individual. But now, they have common blind spots. :-)
StellarScience 4y ago
> a traditional async code review process This is not a very old "tradition." We started code reviews back in the 2000s with in-person small group code reviews, and only later moved to the tool-based async code reviews that are ubiquitous today. We adopted good ideas from a web article "Effective Code Reviews Without the Pain" ( https://www.developer.com/guides/effective-code-reviews-without-the-pain/ https://www.developer.com/guides/effective-code-reviews-with... ). We had folks enter comments in advance but went through them as a group, using social customs like starting questions with "did you consider..." to keep it cordial and non-confrontational. A lot of these reviews generated good discussions about code quality, clarity, etc. We didn't review 100% of code using this process, and we didn't always require the review to be completed before merging - the main goal was more to improve all developers' habits than to improve one particular chunk of code. Moving to the current tool-based async code reviews has pros and cons, but now it takes effort to ensure they stay positive, and the async process eliminated some of the great team discussions that the old-style reviews had. Perhaps the ideal solution is a mix of both approaches?
codeapprove 4y ago
I am biased here since I am the creator of an async code review tool (codeapprove.com) but I think that many teams don't take the time to invest in their code review process and tools enough. For something that most of us do every day (often multiple times per day) there's surprisingly little focus on how to do it well. When I joined my last team they had a bad code review process. Basically once a day or so the CTO and one or two other senior people would look at all the ready-to-merge PRs and give them a thumbs up or thumbs down. I pushed for a much more rigorous but inclusive process. Everyone would do code reviews, every PR would have two approvers, we'd talk about how to do review well and we'd invest in some more automations to solve common pain points. Not only did we get faster as a team (contrary to popular belief, more reviews != slower) but we also got much more thorough feedback AND we were able to consciously propagate the right patterns throughout our codebase in an organic way. It's no exaggeration to say that good code review turned leveled the team more than any other engineering process change we made. More than testing or sprint improvements, more than our management changes, etc. It's highly underrated!
ozim 4y ago
There are multiple good points. Which I would addressed first by tooling of course like linters and automated code checks so low hanging fruit is out of the way. Second thing of shifting left is what I call “developers alignment” - which is yes a meeting where people regularly discuss! things instead of coming up with stuff ad-hoc during review or expecting others to get the context from thin air. Then repeating “obvious” rules regularly because people forget and not expecting people to remember things by heart.

37 points by rekahrv 4y ago

An intriguing article, although my opinion on async reviews is much more positive. :-) There are many arguments for both sides. Pro pair / mob programming: * Faster feedback. * Less time spent on solutions that might be rejected. * Better knowledge sharing (for those who participate). Pro async review: * A more neutral, "external" view. (Smaller chance of groupthink.) * More empathy with future readers of the code. * Encourages to make assumptions and decisions explicit. => Better knowledge sharing for those, who weren't involved in the writing of the code.

ampersandy 4y ago

I don't really buy the two main criticisms around code review. They're stated as intractable downsides but are really just cultural issues around how you perform code reviews. If your coworkers are sending changes for review that don't explain why the changes are being made (and therefore provide the context), reject it until they write a proper summary/test plan that does explain it. You should review the "metadata" of changes before you ever review the code itself and if done well, you'll probably be able to guess what a lot of the incoming changes are before reviewing the code. This makes it easier to spot code that deviates from the stated purpose of the changes and can help identify faults in both understanding of the overall problem or the system being built, especially in more junior engineers. If your coworkers are taking to long to review changes, work with your team to incentivize expedient code reviews, or talk to the people that aren't reviewing enough code to help everyone else get more done. You should also utilize a stack that lets you continue progressing with your work even when waiting for reviews (like stacked diffs).

watwut 4y ago
The context is not just "why the change was made". That point is usually clear from ticket in the first place. The context is "do other places in code use the same tactic" question. It is also the "I tried these four things and three of them failed because of X" or "I had to choose between these two solutions and picked this one".
- ajorgensen 4y ago
  Having the “why” in the same place as the code I think is critically important. Who knows what ticketing system you’ll use in the future, it’s fine to link to but it shouldn’t be where all the context is stored. If you think about a pull request as a story you have who, what, where, when, and why. Most tools handle the first 4 but the why is critically important to anyone, including yourself, who needs to understand the nuance of the change that can’t easily be expressed by the code itself without a novel size comment block. I find that a lot of times people forget the “why” when submitting a change set and simply restate the “what”. At the time you make the change it’s all fresh in your mind and you trick yourself into believing you’ll remember all the details but in reality that’s no my experience.
- onion2k 4y ago
  Do those things matter once the code is actually written though? They're important during the dev process, particularly if you're deciding between two different-but-valid approaches, which is why teams should be communicating and discussing things, but once the decision has been made it ceases to be anything worth worrying about. It's a sunk cost. It shouldn't need to be a part of a code review unless the comms are failing ahead of the review process. If that's the case then you should try to fix the comms issue.
  - taeric 4y ago
    A lot of what we devs think are vitally important... just aren't. Or, they have a steep time decay on how useful they are. I do have the longest code review summaries on my team, amusingly. So, I clearly like the idea of it being a discussion and capturing some of that. I would be challenged to say what part is most important overall, though. In large, because not all code changes are the same.
  - rekahrv 4y ago
    "Do those things matter once the code is actually written though?" That's a great question. I guess it depends on how much the code will change in the future. These things CAN get relevant again when you consider changing that code. E.g. if it turns out later that the library I've chosen isn't maintained anymore. Or we would need some more functionality, but this library doesn't provide it. In these situations, it's useful to see which other alternatives we've considered. That said: I can't really estimate how often this turns out to be relevant, indeed. I guess the answer might be very different for different projects.
rekahrv 4y ago
'You should review the "metadata" of changes before you ever review the code itself' That's a great approach. It sounds logical, but I certainly don't do it consistently. :-)
plorkyeran 4y ago
The first criticism ("Context Gaps. The reviewer doesn’t have the full context of how and why decisions were made for the code they were reviewing.") is one of the things I consider a _advantage_ of code reviews. The article just dismisses the idea that having the reasons for decisions written down is useful, which I find extremely strange.

javaunsafe2019 4y ago

We don’t do code reviews anymore. We have a good quality gate (e2e tests) and pair programming. All development happens on main and gets directly deployed to prod when the tests succeed … I hope I never have to use git flow or alike again in the future!

jagged-chisel 4y ago
What's the size of your team? Remote or on-site? How many timezones does it span? How senior are the team's members?
- sodapopcan 4y ago
  Not the OP but I've worked very successfully pair-programming full-time on a fully remote team of 6 devs, 1 tester, 1 designer, and 1 product manager. All devs were quite senior except for one junior (and that junior leveled up faaaaaasssssst). We usually rotated daily so if a story took three days, the two people who started it weren't the same two who finished it. We all developed a similar coding style for the particular project, everyone knew every part of the codebase to some degree, and we felt a collective ownership over it--there was none of that "Oh, I don't want to change this because Sam wrote it and they'll be mad if I change it" nonsense. However, our timezones only spanned three hours.
- jross225 4y ago
  I'm on a data team of 10, and after about 6 months of onboarding we have permissions to push directly to main pending passing tests. We generally do this for small changes, or changes where we are the context owner. With the caveat that reviews happen after the code is deployed, and usually within a few days. Personally, I like the process. It allows us to move quickly, and focus on blocking changes. We can still get reviews prior to pushing code if it is sensible (for large changes), but most (80%?) changes tend to be quite small.
  - rekahrv 4y ago
    That's an intriguing approach. It makes sense that not all changes are equal and they don't require the same review process. (And it certainly makes sense to encourage small changes.) What qualifies as a "small change"? Do you have some numbers to measure it? Or is it the developer's call?

bsenftner 4y ago

This reads far too basic, like someone read some other blogs and tried to write one without out blatant plagiarism. It's just code advice cliches.

SCdF 4y ago

This article has a "The Issues with Async Reviews" section but not a "The Issues With Pair Programming and Mobbing" one, so it's not a particularly considered piece. I have found that pair programming destroys the ability to coherently think about a large portion of work for a large portion of developers. It is not easy to be "on" all the time, and a lot of people work at a pace or in a structure that is not compatible with that.

sodapopcan 4y ago
I haven't personally found this to be the case at all but pair programming doesn't necessarily mean always working with someone _at all times_. Since I'm a big pairing advocate and I think it's important to pair off every day, but there's absolutely nothing wrong with deciding with your pair that a particular problem needs some alone time.
- deleted 4y ago
  [deleted]

alkonaut 4y ago

The issue with code reviews is that most of them should be prototype reviews before you are even half way done, or design reviews before you even start. Or just dialog around some code or problem, along the way. Looking for quality issues or obvious mistakes after the fact is very rarely useful. I still think it's a good idea to have them as a sign off (e.g. so you at least have 2 people knowing what's happened), but other than that I don't think they provide that much value, at least not for people who are many years into a project and code base. But still we do these after-the-fact code reviews because I don't want to bother my colleague N times in the process. So I churn away until I'm done. And then not to bother them twice, I also polish it. And THEN I ask for a review. The reviewer sees something polished and finished, and assumes that a) since it's polished I must have spent a lot of thought around whether it's the right thing, and obviously anything they can do in the review time which is never going to exceed 2% of the dev time, is rarely going to result in them second guessing that and b) since it's polished they know they'll probably step on some toes if they say "throw it out and do something completely different". They can see the effort. The relationship with a colleague is more important than even the health of the project. So they approve it. The problem of course then is: how do you get from the "too late code reviews that they are rubber stamps" into a process of dialog, half-way reviews and iteration? Other than pair/mob programming I don't know. We say we don't want more processes, but whenever there isn't a process for this iterative behavior, the result is people developing for too long on their own and producing results that are too far gone to be shot down.

lexapro 4y ago
>review time which is never going to exceed 2% of the dev time What, why?
- alkonaut 4y ago
  Because it’s seen as pure overhead, so is expected to take perhaps a few minutes for a large change (that’s my experience at least). The fact that it isn’t is of course not lost on anyone - but everyone accepts the status quo as preferable. That is: people don’t want to spend a lot of time doing code review because they don’t like it. So they rather spend time working on a worse code base.

tomasreimers 4y ago

(I am biased: I am one of the founders of https://graphite.dev https://graphite.dev ) Flip side of this, you can tackle both of their issues with better tooling and not throw the baby out with the bathwater. Time lag: Stacking (https://graphite.dev/stacking https://graphite.dev/stacking) is a powerful technique that allows developers to keep developing even while others read through their code. Context gap: you encounter the same issue in an IDE when exploring a new part of a codebase, and there are tools to help you there (click to see function def, open relevant tests, AI summarize what its doing - we've been trialing the last one internally and it's so cool). Your CR tool should offer all of that, and at many large companies (Google, FB, MSFT, even Jane street) it does. GitHub is just deficient here. I agree with the other comments in this thread that the main problem is that what should have been an architecture / prototype / etc review happens in CR, and I agree with that. But I think moving away from CR worsens that problem not improves on it.

vageli 4y ago
I was intrigued by your post but unfortunately after visiting the site I see no indication of pricing. Since an account is required to submit a stack, I did not proceed without a clear idea of potential cost.
rekahrv 4y ago
"Context gap: you encounter the same issue in an IDE when exploring a new part of a codebase" Yes, that's one of my major arguments for async reviews, especially in large organizations: Future readers of the code will have ca. the same context as the async reviewer. (Contrary to a pair programmer, who has much more context.) "there are tools to help you there" That sounds very interesting. Can you elaborate on that? Which tools do you use when getting familiar with a new part of a codebase? Function def and relevant tests are very useful indeed. AI summary what it's doing. That sounds intriguing. Which tools do you use for that? How do you get the broader context? E.g. how do you decide which other parts of the codebase are closely related (and should be included in your exploration)?

throwawaaarrgh 4y ago

What's wrong with leaving the last hour of the day for team code reviews?

drewcoo 4y ago

Am I the only one who thinks it would be incredibly rude to ask for a code review before I'd actually fleshed out my ideas and also cleaned up to make it presentable? For that matter, if early code review is for design decisions, doesn't that mean tasks are large enough to require plans and reviews of those? Shifting code reviews left seems to be either (or both of): - look, I made a poopie! - let's do Big Planning Up Front (agile has failed)

watwut 4y ago

My biggest issue with code review is that people rely on it and trust it too much. It does certain things in the "better then nothing I guess" way, but gets hyped as if it was all there is to teamwork. Yes, it does little bit to spread knowledge - but it is not much effective at that. Actual occasional architecture session where ideas are explained is much better. Yes, it does little bit to catch bugs, but having actual testing in place does much more. Yes, it does something to teach junior, but for Christ sake, currently it leads to "give them task with no guidance, let them figure it out and then tell them about everything they have done wrong". That is just about the worst way of teaching people. And it creates social issues we don't want to acknowledge, because code review is sacred and cant be criticized. Which leads to kindergarten level of advice to people who run into issues like "dont take it personaly, be nice". Which is next to useless if you have an actual social issue going on in your team.

rekahrv 4y ago
"My biggest issue with code review is that people rely on it and trust it too much." That's a very good way to put it. Code review is useful, but the expectation around it is often over reasonable. I agree on your points regarding quality and knowledge sharing: Code review is beneficial for both, but definitely not sufficient as the only or main tool. "give them task with no guidance, let them figure it out and then tell them about everything they have done wrong" It sounds blatant. But often not far from what's happening. :-) "And it creates social issues we don't want to acknowledge" Yes, that's another longer topic. :-)

larve 4y ago

I think most issues brought up here would be addressed by writing rfcs and have those go through a review cycle, potentially with attached code that is clearly understood to be a proof of concept. It leverages async by giving someone the autonomy to do clear thinking and clear coding, while avoiding pushing the feedback cycle to when all the work has been done and everybody is stuck with a “lgtm can you reindent the last line” review of a fait accompli . Added benefits are proper top level documentation, and a log of the discussions and compromises made to cut short on future design discussions. Edit: typos on mobile

corytheboyd 4y ago
Agreed, the happy path is that code implements a spec that has already been agreed upon, anything less means guess work for both the authors and reviewers of code. Life is about compromise so this doesn't have to be true 100% of the time, but this should be in your team's list of core competencies. Also RFC doesn't HAVE to mean big pedantically formatted and worded monospace document. Templates help make sure common mistakes aren't repeated, but using normal-person English and easy to understand code examples can be very effective.

corytheboyd 4y ago

This is the code review model I've arrived at after going through a number of different processes throughout my career: Code review is a health check from your teammates who don't have the authorship bias. It is not a replacement for testing, both manual and automated. Testing is solely the responsibility of the author, code review is just a design lint check and/or knowledge sharing opportunity. You can still have a QA team of course, but that is usually decoupled from application development and is done post-merge anyway. Every shop is different so YMMV, this is just a personal take.

onion2k 4y ago

Decide if the right structure was chosen: Even if the code is doing the right thing, its structure may be ineffective or inefficient. I can't help thinking that if you're waiting until code review to determine if someone on the team wrote the code in a way that will pass a review or not then you have some serious comms issues on your team. If this is a problem then you should be doing mini reviews during the dev process as well as a code review at the end.

surement 4y ago

pair programming is not a replacement for code reviews; the two people who worked on the code will have the same blind spots (even if the quality of the code is likely going to be better) and a fresh pair of eyes will offer the same benefits as in the regular case

rekahrv 4y ago
Yeah, I agree with that. A pair often produces better code than an individual. But now, they have common blind spots. :-)

StellarScience 4y ago

> a traditional async code review process This is not a very old "tradition." We started code reviews back in the 2000s with in-person small group code reviews, and only later moved to the tool-based async code reviews that are ubiquitous today. We adopted good ideas from a web article "Effective Code Reviews Without the Pain" ( https://www.developer.com/guides/effective-code-reviews-without-the-pain/ https://www.developer.com/guides/effective-code-reviews-with... ). We had folks enter comments in advance but went through them as a group, using social customs like starting questions with "did you consider..." to keep it cordial and non-confrontational. A lot of these reviews generated good discussions about code quality, clarity, etc. We didn't review 100% of code using this process, and we didn't always require the review to be completed before merging - the main goal was more to improve all developers' habits than to improve one particular chunk of code. Moving to the current tool-based async code reviews has pros and cons, but now it takes effort to ensure they stay positive, and the async process eliminated some of the great team discussions that the old-style reviews had. Perhaps the ideal solution is a mix of both approaches?

codeapprove 4y ago

I am biased here since I am the creator of an async code review tool (codeapprove.com) but I think that many teams don't take the time to invest in their code review process and tools enough. For something that most of us do every day (often multiple times per day) there's surprisingly little focus on how to do it well. When I joined my last team they had a bad code review process. Basically once a day or so the CTO and one or two other senior people would look at all the ready-to-merge PRs and give them a thumbs up or thumbs down. I pushed for a much more rigorous but inclusive process. Everyone would do code reviews, every PR would have two approvers, we'd talk about how to do review well and we'd invest in some more automations to solve common pain points. Not only did we get faster as a team (contrary to popular belief, more reviews != slower) but we also got much more thorough feedback AND we were able to consciously propagate the right patterns throughout our codebase in an organic way. It's no exaggeration to say that good code review turned leveled the team more than any other engineering process change we made. More than testing or sprint improvements, more than our management changes, etc. It's highly underrated!

ozim 4y ago

There are multiple good points. Which I would addressed first by tooling of course like linters and automated code checks so low hanging fruit is out of the way. Second thing of shifting left is what I call “developers alignment” - which is yes a meeting where people regularly discuss! things instead of coming up with stuff ad-hoc during review or expecting others to get the context from thin air. Then repeating “obvious” rules regularly because people forget and not expecting people to remember things by heart.