6 ms·

The latter also sounds horrifying; who has time to check all their teams commits? And what sort of motivation (or not!) does that drive into a team :/

valuearb 9y ago
It's horrifying to me that you don't check all of your team's commits. How do they and you improve as developers if you don't discuss how they and you implement things? Most of software development is spent finding and fixing bugs. Doing a little work up front in validating that code is written well and correctly saves a lot of work on the back end.
- toast0 9y ago
  In my team, people have responsibility for their code. If they're not sure about something they can ask. If they think it's fine, they can commit and deploy without waiting for a review. If it blows up, they have to fix it, too. For new people to the team, there's usually some amount of frequent code review to start, but once you have shown you know what you're doing, you can just do it. Knowing if something is likely to cause problems is an important skill, and experience with making bad pushes is probably the best way to learn what is risky and what isn't. (You can and should also learn from other's bad pushes too) I'm a big believer in making it easy to push, because if it's easy to push, it's easy to push fixes, and you don't have to spend a lot of time with prerelease testing. Clearly you can't push a major change to storage libraries on Friday afternoon, but if you can't trust your engineers to make good decisions, you have the wrong engineers.
  - valuearb 9y ago
    I think a significant benefit of my job is that I get to spend part of every day discussing why I or a coworker wrote some code in a specific way. This means I and them are regularly learning and communicating new ways to do things, and improving as engineers. It's also an important part of our QA process. We test our own code, then we test each others code, then we code review each others code. That adds an important layer of quality that we need. In my job, once an app is on the store it's too late, hundreds of thousands of people are going to be using it in the next 24 hours and you can't stop them or push a fix without at least a 24 hour delay in app review.
    - toast0 9y ago
      > In my job, once an app is on the store it's too late, hundreds of thousands of people are going to be using it in the next 24 hours and you can't stop them or push a fix without at least a 24 hour delay in app review. Yes, that's absolutely true -- I'm coming from the server side (mostly). On the client side, there are barriers to pushing that can't feasibly be removed -- in that case, it makes a lot more sense to hunker down and make sure every build is good; some users only rarely update for whatever reason, and you don't want them to be running a bad build for months (or to abandon your product because their first download was a bad build).
  - swift 9y ago
    Not all contributors to a project know all aspects of that project equally well. People also come from a variety of different backgrounds that lends them different insights into the code. Beyond that, it's easy for even experts to make mistakes that they don't recognize in their own work - we've all had the experience of coming back to a piece of writing after a day or two and noticing errors and typos that we couldn't see during the writing process. Waiting for code review is frustrating sometimes (and for that reason, it's important to have a culture of prioritizing code review) but it really helps produce higher quality code, which is a major time-saver in the long run. It also has the side effect of keeping other team members informed about what you're doing, which is important for the overall effectiveness of the team. I agree with you, though, that knowing if things are likely to cause problems is an important skill. If you asked me to give you some examples, not requiring code review would be at the very top of the list. =)
    - toast0 9y ago
      > Not all contributors to a project know all aspects of that project equally well. People also come from a variety of different backgrounds that lends them different insights into the code. Beyond that, it's easy for even experts to make mistakes that they don't recognize in their own work - we've all had the experience of coming back to a piece of writing after a day or two and noticing errors and typos that we couldn't see during the writing process. I expect contributors who are working on an aspect that they're not familiar with to ask for a review; my policy isn't zero reviews, but just like zero reviews is crazy, I find 100% reviews crazy. Experts will make mistakes too, and sometimes a review would have caught it, but you have to think about the cost to the organization for reviewing all changes, along with the cost of mistakes and consider the amount of mistakes that would be found with pre-push reviews. It really depends on the organization, what the cost of mistakes is: I wouldn't be such a cowboy if I were developing self driving cars instead of communications software; it also helps that there are constant failures beyond our control, that need to be handled -- proper handling of those failures often also handles failures within our control, reducing the impact. > I agree with you, though, that knowing if things are likely to cause problems is an important skill. If you asked me to give you some examples, not requiring code review would be at the very top of the list. =) Well, we'd order our lists differently, and have different content in it, but at least we have the same title on our list ;)
  - gfodor 9y ago
    What are the downsides to code review? I don't buy that it is a velocity thing. The only category of changes that I'm willing to accept that may not benefit from code review are small, trivial, low risk changes. (If you think large patches would not benefit from a review process, that's a deeper question but it something if feel 100% to be the case.) But, lucky for us, it turns out that small, trivial, low risk changes are also extremely fast to review. (Like, less than 5 minutes often.) And, haha, sometimes these 'trivial' changes turn out to be not so trivial once a second set of eyes lands on it! The idea that "developers can take responsibility for their changes" is a tautology: if engineers knew up front that a review was unnecessary, then they would know what to expect in the review. But by definition, the point of review is to catch issues and get feedback that are inaccessible to the author. Therefore, it's impossible for an engineer to self-assess if a change requires review: it would mean they could predict the outcome of a review as well. The amount of time a review takes wrt to velocity is a self-regulating function, in my experience. If you are dragged on by a review, it probably means the code warranted review. If the review is rapid, then the review was probably less necessary. But it was fast to do anyway, so doesn't meaningfully impact velocity. Situations like critical bugs and fixes actually are even more important to review, even if they are small patches, because in a incident response situation people are more likely to cut corners, skip steps, or make mistakes due to stress. So what exactly is the point of skipping code review, given that regardless of patch size, there are clear benefits? The benefits are large enough that I consider the burden of proof to be on those who suggest code review be an optional step for deploying changes. The one exception I will raise are externally enforced deadlines, where review is not possible in time due to mitigating factors affecting the reviewer. (For example, the reviewer is a domain expert but is out sick, and the software has a necessary delivery date.) But even in these cases, the review should happen in-full post-deployment, and it should be acknowledged that this is a risk-laden move. And often these are showing secondary flaws in the process (why do we have only one domain expert who could review this? why didn't we have more time allocated for review? etc) In practice, I think a lot of aversion to reviews originate from the frustration of thinking you are finished with a piece of work but having those expectations have to shift after review raises criticism. Everyone wants to ship, especially when they themselves have checked the box that 'it works and I made it as correct as I know how to do.' Addressing reviewer feedback is fundamentally one where you have to draw upon the higher goal of improving code quality and improving your own skills, and like most things that cause real improvements, it can hurt. Honestly, I don't think anyone is free of guilt of feeling this way at one time or another. Beyond that, other fundamental problems can lead to an aversion to code review, such as reviewers missing the forest for the trees, causing reviews to devolve into pedantry, reviewees misjudging the value of others' feedback, mistrust between team members wrt their feedback being valuable, external pressures due to misaligned expectations about delivery, etc. Counterintuitively, strong aversion to code review, an engineering task, can sometimes stem from all kinds of messy, complex problems with team dynamics. So keep in mind that if people are continually complaining about code reviews, this might be telling you something much deeper going on! I think in general a solid code review from a peer you trust is one of these things that generally speaking, hurts in the way pushing yourself in exercise does. I personally feel it fosters a kind of skill growth you cannot get otherwise, except perhaps through deliberate practice. In other words, no pain, no gain.
  - nradov 9y ago
    Sure that's fine if the applications you're working on aren't mission-critical, and if the potential consequences for failure don't involve safety risks, large financial losses, or unrecoverable data corruption. Some of us have to be a little more careful with quality control. Also a good team should have collective code ownership. No single person can be responsible for any piece of code. What happens when that person is out on vacation?
- ErrantX 9y ago
  Does that require checking all commits though? That sounds like micro-management which is a massive morale killer. Also; the team should self-organise on quality and help each other improve. I set the quality expectation and get enough data to understand their progress against that expectation (FWIW: I've always found a teams expectations sit higher than mine - which is as it should be to help motivation).
  - patmcc 9y ago
    >>>That sounds like micro-management which is a massive morale killer. I could see that if all commits go through your boss or something, but lots of places do peer-based code review. So have another developer check on your stuff and you check on theirs (or round robin it, or two on one, or whatever). This helps in a lot of ways - maybe they'll catch something wrong, maybe it'll trigger a good idea in them, maybe they'll just be more familiar with a part of the code they hadn't seen much of before. And it honestly doesn't take that long; code that can be written in an hour can usually be checked in five or ten minutes.
    - ErrantX 9y ago
      Yes I suspect that's the misunderstanding; I was approaching this from a "boss" perspective, not a peer one. Peer-based reviews are critical.

valuearb 9y ago

It's horrifying to me that you don't check all of your team's commits. How do they and you improve as developers if you don't discuss how they and you implement things? Most of software development is spent finding and fixing bugs. Doing a little work up front in validating that code is written well and correctly saves a lot of work on the back end.

toast0 9y ago
In my team, people have responsibility for their code. If they're not sure about something they can ask. If they think it's fine, they can commit and deploy without waiting for a review. If it blows up, they have to fix it, too. For new people to the team, there's usually some amount of frequent code review to start, but once you have shown you know what you're doing, you can just do it. Knowing if something is likely to cause problems is an important skill, and experience with making bad pushes is probably the best way to learn what is risky and what isn't. (You can and should also learn from other's bad pushes too) I'm a big believer in making it easy to push, because if it's easy to push, it's easy to push fixes, and you don't have to spend a lot of time with prerelease testing. Clearly you can't push a major change to storage libraries on Friday afternoon, but if you can't trust your engineers to make good decisions, you have the wrong engineers.
- valuearb 9y ago
  I think a significant benefit of my job is that I get to spend part of every day discussing why I or a coworker wrote some code in a specific way. This means I and them are regularly learning and communicating new ways to do things, and improving as engineers. It's also an important part of our QA process. We test our own code, then we test each others code, then we code review each others code. That adds an important layer of quality that we need. In my job, once an app is on the store it's too late, hundreds of thousands of people are going to be using it in the next 24 hours and you can't stop them or push a fix without at least a 24 hour delay in app review.
  - toast0 9y ago
    > In my job, once an app is on the store it's too late, hundreds of thousands of people are going to be using it in the next 24 hours and you can't stop them or push a fix without at least a 24 hour delay in app review. Yes, that's absolutely true -- I'm coming from the server side (mostly). On the client side, there are barriers to pushing that can't feasibly be removed -- in that case, it makes a lot more sense to hunker down and make sure every build is good; some users only rarely update for whatever reason, and you don't want them to be running a bad build for months (or to abandon your product because their first download was a bad build).
- swift 9y ago
  Not all contributors to a project know all aspects of that project equally well. People also come from a variety of different backgrounds that lends them different insights into the code. Beyond that, it's easy for even experts to make mistakes that they don't recognize in their own work - we've all had the experience of coming back to a piece of writing after a day or two and noticing errors and typos that we couldn't see during the writing process. Waiting for code review is frustrating sometimes (and for that reason, it's important to have a culture of prioritizing code review) but it really helps produce higher quality code, which is a major time-saver in the long run. It also has the side effect of keeping other team members informed about what you're doing, which is important for the overall effectiveness of the team. I agree with you, though, that knowing if things are likely to cause problems is an important skill. If you asked me to give you some examples, not requiring code review would be at the very top of the list. =)
  - toast0 9y ago
    > Not all contributors to a project know all aspects of that project equally well. People also come from a variety of different backgrounds that lends them different insights into the code. Beyond that, it's easy for even experts to make mistakes that they don't recognize in their own work - we've all had the experience of coming back to a piece of writing after a day or two and noticing errors and typos that we couldn't see during the writing process. I expect contributors who are working on an aspect that they're not familiar with to ask for a review; my policy isn't zero reviews, but just like zero reviews is crazy, I find 100% reviews crazy. Experts will make mistakes too, and sometimes a review would have caught it, but you have to think about the cost to the organization for reviewing all changes, along with the cost of mistakes and consider the amount of mistakes that would be found with pre-push reviews. It really depends on the organization, what the cost of mistakes is: I wouldn't be such a cowboy if I were developing self driving cars instead of communications software; it also helps that there are constant failures beyond our control, that need to be handled -- proper handling of those failures often also handles failures within our control, reducing the impact. > I agree with you, though, that knowing if things are likely to cause problems is an important skill. If you asked me to give you some examples, not requiring code review would be at the very top of the list. =) Well, we'd order our lists differently, and have different content in it, but at least we have the same title on our list ;)
- gfodor 9y ago
  What are the downsides to code review? I don't buy that it is a velocity thing. The only category of changes that I'm willing to accept that may not benefit from code review are small, trivial, low risk changes. (If you think large patches would not benefit from a review process, that's a deeper question but it something if feel 100% to be the case.) But, lucky for us, it turns out that small, trivial, low risk changes are also extremely fast to review. (Like, less than 5 minutes often.) And, haha, sometimes these 'trivial' changes turn out to be not so trivial once a second set of eyes lands on it! The idea that "developers can take responsibility for their changes" is a tautology: if engineers knew up front that a review was unnecessary, then they would know what to expect in the review. But by definition, the point of review is to catch issues and get feedback that are inaccessible to the author. Therefore, it's impossible for an engineer to self-assess if a change requires review: it would mean they could predict the outcome of a review as well. The amount of time a review takes wrt to velocity is a self-regulating function, in my experience. If you are dragged on by a review, it probably means the code warranted review. If the review is rapid, then the review was probably less necessary. But it was fast to do anyway, so doesn't meaningfully impact velocity. Situations like critical bugs and fixes actually are even more important to review, even if they are small patches, because in a incident response situation people are more likely to cut corners, skip steps, or make mistakes due to stress. So what exactly is the point of skipping code review, given that regardless of patch size, there are clear benefits? The benefits are large enough that I consider the burden of proof to be on those who suggest code review be an optional step for deploying changes. The one exception I will raise are externally enforced deadlines, where review is not possible in time due to mitigating factors affecting the reviewer. (For example, the reviewer is a domain expert but is out sick, and the software has a necessary delivery date.) But even in these cases, the review should happen in-full post-deployment, and it should be acknowledged that this is a risk-laden move. And often these are showing secondary flaws in the process (why do we have only one domain expert who could review this? why didn't we have more time allocated for review? etc) In practice, I think a lot of aversion to reviews originate from the frustration of thinking you are finished with a piece of work but having those expectations have to shift after review raises criticism. Everyone wants to ship, especially when they themselves have checked the box that 'it works and I made it as correct as I know how to do.' Addressing reviewer feedback is fundamentally one where you have to draw upon the higher goal of improving code quality and improving your own skills, and like most things that cause real improvements, it can hurt. Honestly, I don't think anyone is free of guilt of feeling this way at one time or another. Beyond that, other fundamental problems can lead to an aversion to code review, such as reviewers missing the forest for the trees, causing reviews to devolve into pedantry, reviewees misjudging the value of others' feedback, mistrust between team members wrt their feedback being valuable, external pressures due to misaligned expectations about delivery, etc. Counterintuitively, strong aversion to code review, an engineering task, can sometimes stem from all kinds of messy, complex problems with team dynamics. So keep in mind that if people are continually complaining about code reviews, this might be telling you something much deeper going on! I think in general a solid code review from a peer you trust is one of these things that generally speaking, hurts in the way pushing yourself in exercise does. I personally feel it fosters a kind of skill growth you cannot get otherwise, except perhaps through deliberate practice. In other words, no pain, no gain.
- nradov 9y ago
  Sure that's fine if the applications you're working on aren't mission-critical, and if the potential consequences for failure don't involve safety risks, large financial losses, or unrecoverable data corruption. Some of us have to be a little more careful with quality control. Also a good team should have collective code ownership. No single person can be responsible for any piece of code. What happens when that person is out on vacation?
ErrantX 9y ago
Does that require checking all commits though? That sounds like micro-management which is a massive morale killer. Also; the team should self-organise on quality and help each other improve. I set the quality expectation and get enough data to understand their progress against that expectation (FWIW: I've always found a teams expectations sit higher than mine - which is as it should be to help motivation).
- patmcc 9y ago
  >>>That sounds like micro-management which is a massive morale killer. I could see that if all commits go through your boss or something, but lots of places do peer-based code review. So have another developer check on your stuff and you check on theirs (or round robin it, or two on one, or whatever). This helps in a lot of ways - maybe they'll catch something wrong, maybe it'll trigger a good idea in them, maybe they'll just be more familiar with a part of the code they hadn't seen much of before. And it honestly doesn't take that long; code that can be written in an hour can usually be checked in five or ten minutes.
  - ErrantX 9y ago
    Yes I suspect that's the misunderstanding; I was approaching this from a "boss" perspective, not a peer one. Peer-based reviews are critical.