6 ms·

This entirely misses the point of code review. Say it with me: Code review is a knowledge transfer exercise. Finding bugs, security vulnerabilities, and keepi

This entirely misses the point of code review. Say it with me: Code review is a knowledge transfer exercise. Finding bugs, security vulnerabilities, and keeping the code maintainable are merely side effects that we appreciate along the way. The primary purpose of code review is increasing the bus factor of the given piece of code and facilitating organic knowledge transfer. That's it.

SomeHacker44 5y ago
Maybe for you. For others it can have all or any of those other things as its primary purpose. One important thing for me is that it prevents or at least mitigates unilateral insider attacks by having two people required to change code.
- astrobe_ 5y ago
  Having a bus factor>1 is still a major prerequisite for doing that. Because if you have a junior developer review the changes of a senior developer, there are so many social engineering tricks to make the backdoor pass the review that it isn't even funny.
pelasaco 5y ago
"but they have AI".. probably not. Probably the same linters and scanners that we all know + cheap working force reviewing code manually.
mfashby 5y ago
Hmm, what about education? I've learned a bunch through thoughtful reviews of my code by other devs.
- loloquwowndueo 5y ago
  That’s them transferring their knowledge to you :)
beanjuiceII 5y ago
i thought code review was to assert dominance and to block as long as possible for yak shaving reasons; But I do like your version of it! Happy holidays~
vinceguidry 5y ago
I mentioned this idea to a very knowledgeable enterprise architect. I was mentioning how much I'd enjoyed being able to use the code review process in this way. His reply: As good of an idea that is, it breaks Agile. So long as the code meets the biz-provided spec, it must be accepted, and if there are other concerns with the code, to make a tech debt ticket to address it at a later date. This, of course, horrified me.
- haggy102 5y ago
  Anyone with Architect in their title receives skepticism from me. I've turned down title changes that include it and I refuse to put it on my LinkedIn.
  - akvadrako 5y ago
    Even people designing buildings?
  - vinceguidry 5y ago
    His world seemed completely and utterly divorced from mine. He was deeply connected with big enterprises and how they manage to meet business objectives regardless of not being software engineers themselves. In our world, we take for granted that our managers and leadership are technically proficient. In his, the "Engineering Manager" is a rarity, the people managing engineers are just ordinary managers. Software architecture, in this world, is how you escape the rat race of soulless ticket punching without having to go into management. You use your skills and experience in an advisory role. Obviously there are better or worse architects, I've had the pleasure of working with really good ones. But it often feels like a title and field borne out of a need to retain top talent. (read, not push them away to competitors)
  - dangus 5y ago
    I think that attitude is a little short-sighted, and perhaps a little bit of "anyone that isn't a pure developer is incompetent" elitism. For one thing, titles are just titles, and they stick around for historic reasons. For example, there really isn't a great reason for companies to call people DevOps Engineer, but it still happens. I'll take the Solutions Architect role as an example. It's basically sales or customer-oriented developer or technical resource that works with customers to determine what a solution to a problem will look like. "Architect" is mostly meaningless in the sense that Solutions Architects don't really architect much of anything. Usually, they just come up with a plausible path forward and visualize that solution to all the stakeholders involved. This includes travel to customer sites, something that developers are basically never willing or expected to do. They're the folks who deal with the god damn customers so the engineers don't have to. They have people skills, they're good at dealing with people. Yeah, I mean, considering the staff engineer on my team does not shower, there is value in that role. Most of the value in the Solutions Architect is how they're able to work with customers to discover their needs on a more technical level rather than at a high level. Once the plan is determined, the solutions architects don't actually build the solution on their own, they're more like an interface or leader to the development team that builds the solution. https://www.careerexplorer.com/careers/solution-architect/ https://www.careerexplorer.com/careers/solution-architect/ I don't want to put words into your mouth, but maybe you're skeptical of "architects" because they aren't typically expert specialists in one small area. Maybe that's why you don't want to be associate with them. Understandable, perhaps, but don't be misguided into thinking that "architects" aren't skilled professionals who add value to the company. On top of that, I believe they're often paid higher than developers ;-)
senko 5y ago
So, they find bugs and security vulnerabilities, help spot performance problems and keep the code maintainable -- as a service. Still sounds like a pretty valuable service.
nerdjon 5y ago
Personally I work on a 2 person team, the vast majority of my reviews either have no changes or "hey maybe this thing should be named something else so its more consistent". For us the peer review is almost completely for knowledge transfer. Sure we know what each other is working on, but we still have to maintain each others code if something goes wrong and the other is not available. So I agree with this 100%. Even in bigger organizations where the peer review was more formal, I feel like that is still the primary goal. I do fail to see the benefit of this, especially looking at the price of it. Outside of maybe scripts? I can't really see how much they can actually help without the context of the larger application. Without that context can they really provide more benefit than AWS CodeGuru (or similar) could offer?
- watwut 5y ago
  We do have detailed code reviews. People will complain about lines you write and expect a lot of consistency. Simultaneously, architecture is complete mess, because architecture is much harder to see in code review.
walkingriver 5y ago
In most of the projects I have reviewed for pullrequest.com, the engineering team is also doing its own reviews. We are "another set of eyes" as it were. Many of our larger reviews can end up becoming lengthy conversations between us and the team. There is definitely a lot of knowledge transfer going on. What's been truly rewarding is when someone on the team comes back to us and asks for advice on the best way to solve a particular problem.
chillingeffect 5y ago
> Say it with me: Code review is a knowledge transfer exercise. It is, but (say it with me?): Things change. Code review is not the sole knowledge transfer exercise, nor should it be forever. You could say similar things about "make format". Now that our formatting is automatic, we can discuss code at a higher level. If code reviews were standardized or even automated, we could discuss it at an even higher level.
ivanhoe 5y ago
Finding bugs is of questionable use because it's sort of limited to obvious bloopers. One usually needs to be deeply involved and familiar with the business logic to be able to spot a real bug of the type that will give you serious trouble - external services obviously will never have time for such commitment. That said, I think there's still a lot of space where this service can be very useful: from improving your code style and an affordable way to have security audits, to just a support net for developers who might feel overwhelmed by a task sometimes, and could use some friendly advices from a seasoned dev. It being an external service can also make it less stressful and personal, which is great as some devs see code reviews as a criticism of their skills and go all defensive about it, creating tensions in teams (sounds silly, but I've seen a lot of it).
Pxtl 5y ago
This. Code review is a terrible way to find bugs... At best you'll get the more Senior reviewer to spot library uses that are known to be workable but problematic (eg poor performance), coming from their experience. But straight up logic bugs are hard to spot. The big thing that code review achieves is that it ensures a 2nd person understands the code that was written, and therefore it is possible for the reader to understand. So 3 years down the road and you're looking at some counterintuitive piece of code, the reader isn't wondering *why on earth did he write it like this? Is it working around some cryptic edge-case bug in the framework or were they just stupid?"
gwbas1c 5y ago
> Code review is a knowledge transfer exercise In many places, yes. But: When you have a solo developer working on a component in a language that you aren't familiar with, team full of novices, component delivered by a contractor...
tantalor 5y ago
You're thinking of pair programming. That's different.
Jach 5y ago
The case for code review is fundamentally economical: it saves the organization money by finding costly issues earlier when they are cheaper to fix without imposing a costlier burden for the review process itself. As generic "knowledge transfer", or even increasing the bus factor, I would disagree. The best knowledge transfer experiences I've had have been dedicated meetings/workgroups dedicated to that purpose, and they tend to encompass larger scopes than a single commit/pull request. I've also seen the difference in devs who contribute to an area having previously only been reviewers of code in that area, vs. actually having a knowledge transfer session with that area's lead beforehand, and in the latter case they are more effective (actually even if they had never reviewed code in that area before, as was the case with interns or new hires). To me knowledge transfer is the nice possible side effect, but not the primary purpose. I'll leave a third opinion from here https://static1.smartbear.co/smartbear/media/pdfs/best-kept-secrets-of-peer-code-review_redirected.pdf#page=16 https://static1.smartbear.co/smartbear/media/pdfs/best-kept-... which lists some direct and indirect benefits: Few developers and software project managers would argue that these are direct benefits of conducting code reviews: • Improved code quality • Fewer defects in code • Improved communication about code content • Education of junior programmers And these indirect benefits are byproducts of code review: • Shorter development/test cycles • Reduced impact on technical support • More customer satisfaction • More maintainable code
dpweb 5y ago
Not exactly. imo and just a perspective, but code review is one method of KT. Sending someone a document can also be a method of KT. Code review for KT is one thing. Code review for finding bugs is another. Code review for following style guidelines is yet another. I would like to use a baseline style guideline for JS is anyone aware of one that isn't too huge?
- NateEag 5y ago
  I'm a fan of using code formatters to define how you lay out code in a file. Prettier is popular for that job: https://prettier.io/ https://prettier.io/ For detecting functional/idiomatic/behavioral issues, ESLint is my go-to: https://eslint.org/ https://eslint.org/ This shows my bias for automation over human enforcement.
pictur 5y ago
code review is a process that carries a lot of meaning today. the work cycle in most workplaces is not qualified to meet this meaning.
sbassi 5y ago
Some companies doesn't have enough people to review the code, at least not with the required seniority. This way there is also knowledge transfer, but to the company. And with people not in your payroll that can provide objective comments since they are not afraid of telling the wrong person that their code sucks.
oneepic 5y ago
Even without hard stats and evidence, I guarantee most people don't think of code review as mostly being about KT. The purpose(s) of code review commonly contain the ones you described, but it varies from team to team. The most common I'd guess is putting a 2nd pair of eyes on your code, checking code quality and finding issues.

SomeHacker44 5y ago

Maybe for you. For others it can have all or any of those other things as its primary purpose. One important thing for me is that it prevents or at least mitigates unilateral insider attacks by having two people required to change code.

astrobe_ 5y ago
Having a bus factor>1 is still a major prerequisite for doing that. Because if you have a junior developer review the changes of a senior developer, there are so many social engineering tricks to make the backdoor pass the review that it isn't even funny.

pelasaco 5y ago

"but they have AI".. probably not. Probably the same linters and scanners that we all know + cheap working force reviewing code manually.

mfashby 5y ago

Hmm, what about education? I've learned a bunch through thoughtful reviews of my code by other devs.

loloquwowndueo 5y ago
That’s them transferring their knowledge to you :)

beanjuiceII 5y ago

i thought code review was to assert dominance and to block as long as possible for yak shaving reasons; But I do like your version of it! Happy holidays~

vinceguidry 5y ago

I mentioned this idea to a very knowledgeable enterprise architect. I was mentioning how much I'd enjoyed being able to use the code review process in this way. His reply: As good of an idea that is, it breaks Agile. So long as the code meets the biz-provided spec, it must be accepted, and if there are other concerns with the code, to make a tech debt ticket to address it at a later date. This, of course, horrified me.

haggy102 5y ago
Anyone with Architect in their title receives skepticism from me. I've turned down title changes that include it and I refuse to put it on my LinkedIn.
- akvadrako 5y ago
  Even people designing buildings?
- vinceguidry 5y ago
  His world seemed completely and utterly divorced from mine. He was deeply connected with big enterprises and how they manage to meet business objectives regardless of not being software engineers themselves. In our world, we take for granted that our managers and leadership are technically proficient. In his, the "Engineering Manager" is a rarity, the people managing engineers are just ordinary managers. Software architecture, in this world, is how you escape the rat race of soulless ticket punching without having to go into management. You use your skills and experience in an advisory role. Obviously there are better or worse architects, I've had the pleasure of working with really good ones. But it often feels like a title and field borne out of a need to retain top talent. (read, not push them away to competitors)
- dangus 5y ago
  I think that attitude is a little short-sighted, and perhaps a little bit of "anyone that isn't a pure developer is incompetent" elitism. For one thing, titles are just titles, and they stick around for historic reasons. For example, there really isn't a great reason for companies to call people DevOps Engineer, but it still happens. I'll take the Solutions Architect role as an example. It's basically sales or customer-oriented developer or technical resource that works with customers to determine what a solution to a problem will look like. "Architect" is mostly meaningless in the sense that Solutions Architects don't really architect much of anything. Usually, they just come up with a plausible path forward and visualize that solution to all the stakeholders involved. This includes travel to customer sites, something that developers are basically never willing or expected to do. They're the folks who deal with the god damn customers so the engineers don't have to. They have people skills, they're good at dealing with people. Yeah, I mean, considering the staff engineer on my team does not shower, there is value in that role. Most of the value in the Solutions Architect is how they're able to work with customers to discover their needs on a more technical level rather than at a high level. Once the plan is determined, the solutions architects don't actually build the solution on their own, they're more like an interface or leader to the development team that builds the solution. https://www.careerexplorer.com/careers/solution-architect/ https://www.careerexplorer.com/careers/solution-architect/ I don't want to put words into your mouth, but maybe you're skeptical of "architects" because they aren't typically expert specialists in one small area. Maybe that's why you don't want to be associate with them. Understandable, perhaps, but don't be misguided into thinking that "architects" aren't skilled professionals who add value to the company. On top of that, I believe they're often paid higher than developers ;-)

senko 5y ago

So, they find bugs and security vulnerabilities, help spot performance problems and keep the code maintainable -- as a service. Still sounds like a pretty valuable service.

nerdjon 5y ago

Personally I work on a 2 person team, the vast majority of my reviews either have no changes or "hey maybe this thing should be named something else so its more consistent". For us the peer review is almost completely for knowledge transfer. Sure we know what each other is working on, but we still have to maintain each others code if something goes wrong and the other is not available. So I agree with this 100%. Even in bigger organizations where the peer review was more formal, I feel like that is still the primary goal. I do fail to see the benefit of this, especially looking at the price of it. Outside of maybe scripts? I can't really see how much they can actually help without the context of the larger application. Without that context can they really provide more benefit than AWS CodeGuru (or similar) could offer?

watwut 5y ago
We do have detailed code reviews. People will complain about lines you write and expect a lot of consistency. Simultaneously, architecture is complete mess, because architecture is much harder to see in code review.

walkingriver 5y ago

In most of the projects I have reviewed for pullrequest.com, the engineering team is also doing its own reviews. We are "another set of eyes" as it were. Many of our larger reviews can end up becoming lengthy conversations between us and the team. There is definitely a lot of knowledge transfer going on. What's been truly rewarding is when someone on the team comes back to us and asks for advice on the best way to solve a particular problem.

chillingeffect 5y ago

> Say it with me: Code review is a knowledge transfer exercise. It is, but (say it with me?): Things change. Code review is not the sole knowledge transfer exercise, nor should it be forever. You could say similar things about "make format". Now that our formatting is automatic, we can discuss code at a higher level. If code reviews were standardized or even automated, we could discuss it at an even higher level.

ivanhoe 5y ago

Finding bugs is of questionable use because it's sort of limited to obvious bloopers. One usually needs to be deeply involved and familiar with the business logic to be able to spot a real bug of the type that will give you serious trouble - external services obviously will never have time for such commitment. That said, I think there's still a lot of space where this service can be very useful: from improving your code style and an affordable way to have security audits, to just a support net for developers who might feel overwhelmed by a task sometimes, and could use some friendly advices from a seasoned dev. It being an external service can also make it less stressful and personal, which is great as some devs see code reviews as a criticism of their skills and go all defensive about it, creating tensions in teams (sounds silly, but I've seen a lot of it).

Pxtl 5y ago

This. Code review is a terrible way to find bugs... At best you'll get the more Senior reviewer to spot library uses that are known to be workable but problematic (eg poor performance), coming from their experience. But straight up logic bugs are hard to spot. The big thing that code review achieves is that it ensures a 2nd person understands the code that was written, and therefore it is possible for the reader to understand. So 3 years down the road and you're looking at some counterintuitive piece of code, the reader isn't wondering *why on earth did he write it like this? Is it working around some cryptic edge-case bug in the framework or were they just stupid?"

gwbas1c 5y ago

> Code review is a knowledge transfer exercise In many places, yes. But: When you have a solo developer working on a component in a language that you aren't familiar with, team full of novices, component delivered by a contractor...

tantalor 5y ago

You're thinking of pair programming. That's different.

Jach 5y ago

The case for code review is fundamentally economical: it saves the organization money by finding costly issues earlier when they are cheaper to fix without imposing a costlier burden for the review process itself. As generic "knowledge transfer", or even increasing the bus factor, I would disagree. The best knowledge transfer experiences I've had have been dedicated meetings/workgroups dedicated to that purpose, and they tend to encompass larger scopes than a single commit/pull request. I've also seen the difference in devs who contribute to an area having previously only been reviewers of code in that area, vs. actually having a knowledge transfer session with that area's lead beforehand, and in the latter case they are more effective (actually even if they had never reviewed code in that area before, as was the case with interns or new hires). To me knowledge transfer is the nice possible side effect, but not the primary purpose. I'll leave a third opinion from here https://static1.smartbear.co/smartbear/media/pdfs/best-kept-secrets-of-peer-code-review_redirected.pdf#page=16 https://static1.smartbear.co/smartbear/media/pdfs/best-kept-... which lists some direct and indirect benefits: Few developers and software project managers would argue that these are direct benefits of conducting code reviews: • Improved code quality • Fewer defects in code • Improved communication about code content • Education of junior programmers And these indirect benefits are byproducts of code review: • Shorter development/test cycles • Reduced impact on technical support • More customer satisfaction • More maintainable code

dpweb 5y ago

Not exactly. imo and just a perspective, but code review is one method of KT. Sending someone a document can also be a method of KT. Code review for KT is one thing. Code review for finding bugs is another. Code review for following style guidelines is yet another. I would like to use a baseline style guideline for JS is anyone aware of one that isn't too huge?

NateEag 5y ago
I'm a fan of using code formatters to define how you lay out code in a file. Prettier is popular for that job: https://prettier.io/ https://prettier.io/ For detecting functional/idiomatic/behavioral issues, ESLint is my go-to: https://eslint.org/ https://eslint.org/ This shows my bias for automation over human enforcement.

pictur 5y ago

code review is a process that carries a lot of meaning today. the work cycle in most workplaces is not qualified to meet this meaning.

sbassi 5y ago

Some companies doesn't have enough people to review the code, at least not with the required seniority. This way there is also knowledge transfer, but to the company. And with people not in your payroll that can provide objective comments since they are not afraid of telling the wrong person that their code sucks.

oneepic 5y ago

Even without hard stats and evidence, I guarantee most people don't think of code review as mostly being about KT. The purpose(s) of code review commonly contain the ones you described, but it varies from team to team. The most common I'd guess is putting a 2nd pair of eyes on your code, checking code quality and finding issues.