12 ms·

What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't

by 3036e4 1y ago

What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have been done at all. It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. There may be some discussion earlier on in a jira ticket or meeting, and with some luck someone even wrote a design spec, but there will still often be someone from a different team or distant part of the organization that only hears about the change when they see the code review. This includes me. I often only notice that some other team implemented something stupid because I suddenly get a notification that someone posted a code review for some part of the code that I watch for changes. Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? I don't know. That doesn't seem to ever happen anyway. At university in the 1990's in a course about development processes there wasn't only code reviews but also design reviews, and that isn't something I ever encountered in the wild (in any formal sense) but I don't know if even a design review process would be able to catch all the things you would want to catch BEFORE starting to implement something.

epolanski 1y ago
> and that isn't something I ever encountered in the wild (in any formal sense) Because in the software engineering world there is very little engineering involved. That being said, I also think that the industry is unwilling to accept the slowliness of the proper engineering process for various reasons, including non criticality of most software and the possibility to amend bugs and errors on the fly. Other engineering fields enjoy no such luxuries, the bridge either holds the train or it doesn't, you either nailed the manufacturing plant or there's little room for fixing, the plane's engine either works or not Different stakes and patching opportunities lend to different practices.
- ozim 1y ago
  It still is engineering you only mistake design phase. Writing code is the design phase. You don’t need design phase for doing design. Will drop link to relevant video later.
  - ozim 1y ago
    This is the talk on real software engineering: https://www.youtube.com/watch?v=RhdlBHHimeM https://www.youtube.com/watch?v=RhdlBHHimeM
  - jmholla 1y ago
    I also read this series of blog posts recently where the author, Hillel Wayne, talked to several "traditional" engineers that had made the switch to software. He came to a similar conclusion and while I was previously on the fence of how much of what software developers do could be considered engineering, it convinced me that software engineer is a valid title and that what we do is engineering. First post here: https://www.hillelwayne.com/post/are-we-really-engineers/ https://www.hillelwayne.com/post/are-we-really-engineers/
    - 3036e4 1y ago
      I was an undergraduate (computer) engineer student, but like many of my friends at that time (dot-com boom) I did not graduate since it was too tempting to get a job and get well paid instead. However many, probably half, that I work with, and most that I worked with overall for the last 25+ years (since after I dropped out) have an engineering degree. Especially the younger ones, since this century it has been more focus on getting a degree and fewer seems to drop out early to get a job like many of us did in my days. So when American employers insist on giving me titles like "software engineer" I cringe. It's embarrassing really, since I am surrounded by so many that have a real engineering degree, and I don't. It's like if I dropped out of medical school and then people started calling me "doctor" even if I wasn't one, legally. It would be amazing if we could find a better word so that non-engineers like me are not confused with the legally real engineers.
teiferer 1y ago
Seems like your organization is lacking structure and communication. Where I work, the structure is such that most parts of the codebase have a team that is responsible for it and does the vast majority of changes there. If any "outsider" plans a change, they come talk to the team and coordinates. And we also have strong intra-team communication. It's clear who is working on what and we have design reviews to agree on the "how" within the team. It's rare that what you describe happens. 95% of the code reviews I do are without comments or only with minor suggestions for improvement. Mainly because we have developed a culture of talking to each other about major things beforehand and writing the code is really just the last step in the process. We also have developed a somewhat consistent style within the teams. Not necessarily across the teams, but that's ok. TL;DR: It's certainly possible to do things better that what you are experiencing. It's a matter of structure, communication and culture.
DixieDev 1y ago
Where I work we tend to write RFCs for fundamental design decisions. Deciding what counts as a "fundamental design decision" is sometimes self-moderated in the moment but we also account for it when making long term plans. For example when initially creating epics in Jira we might find it hard to flesh out as we don't really know how we're going to approach it, so we just start it off with a task to write an RFC. These can be written either for just our team or for the eyes of all other software teams. In the latter case we put these forward as RFCs for discussion in a fortnightly meeting, which is announced well in advance so people can read them, leave comments beforehand, and only need to attend the meeting if there's an RFC of interest to them up for discussion. This has gone pretty well for us! It can feel like a pain to write some of these, and at times I think we overuse them somewhat, but I much prefer our approach to any other place I've worked where we didn't have any sort of collaborative design process in place at all.
- unkulunkulu 1y ago
  I view this process like this: code review is a communication tool: you can discuss concrete decisions vs hand waving and explaining in the conceptual space, which of course has its place, but is limited. But writing the whole working code just to discuss some APIs is too much and will require extra work to change if problems are surfaced on review. So a design document is something in the middle: it should draw a line where the picture of the planned change is as clear as possible and can be communicated with shareholders. Other possible middle grounds include PRs that don’t pass all tests or that don’t even build at all. You just have to choose the most appropriate sequence of communication tools to come to agreements in the team and come to a point where the team is on the same page on all the decisions and how the final picture looks.
skybrian 1y ago
One way to fix it: pair programming. You're getting feedback in real time as you write the code. Unfortunately, the conditions where it works well can be difficult to set up. You need people who are into it and have similar schedules. And you don't want two people waiting for tests to run.
- grilledcheez 1y ago
  It's crazy that we go out of our way to design processes (code review, design review) to avoid actually just... working together? And then we design organizations that optimize for those processes instead of optimizing for collaboration.
- guytv 1y ago
  Anyone actually done long-term pair programming and lived to tell the tale? Is it real, or just a utopian fantasy?
  - skybrian 1y ago
    I did it at a startup for a few months. The startup failed, but I think it was more of a business failure. Pivotal Labs was a contracting firm that did it for years. They aren’t around anymore, but they had a good run: https://en.m.wikipedia.org/wiki/Pivotal_Labs https://en.m.wikipedia.org/wiki/Pivotal_Labs
  - jacques_chester 1y ago
    It's not for everyone. Some people have excellent reasons why it isn't workable for them. Others have had terrible experiences. It takes a great deal of practice to be a good pair and, if you don't start by working with an experienced pair, your memories of pairing are unlikely to be fond. However. I paired full-time, all day, at Pivotal, for 5 years. It was incredible. Truly amazing. The only time in my career when I really thrived. I miss it badly.
Ozzie_osman 1y ago
> Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? You don't need to. I've seen this generally work with some mix of the following: 1. Try to decouple systems so that it's less likely for someone in a part of the org to make changes that negatively impact someone in a more distant part of the org. 2. Some design review process: can be formal "you will write a design doc, it will be reviewed and formally approved in a design committee" if you care more about integrity and less about speed, or can be "write a quick RFC document and share it to the relevant team(s)". 3. Some group of people that have broad context on the system/code-base (usually more senior or tenured engineers). Again, can be formal: "here is the design review committee" or less formal: "run it by these set of folks who know there stuff". If done well, I'd say you can get pretty broad coverage from a group like this. Definitely not "everyone in the entire company". That group can also redirect or pull others in. 4. Accept that the process will be a bit lossy. Not just because you may miss a reviewer, but also, because sometimes once you start implementing the reality of implementation is different than what people expect. You can design the process for this by encouraging POC or draft implementations or spikes, and set expectations that not all code is expected to make it into production (any creative process includes drafts, rewrites, etc that may not be part of the final form, but help explore the best final form). I've basically seen this work pretty well at company sizes from 5 engineers all the way up to thousands.
motorest 1y ago
> It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. That is a problem with your organization, not with Git or any version control system. PRs are orthogonal to it. If you drop by a PR without being aware of the ticket that made the PR happen and the whole discussion and decision process that led to the creation of said tickets, you are out of the loop. Your complain is like a book publisher complaining that the printing process is flawed because seeing the printed book coming out of the production line is the only time when all stakeholders really get involved. Only if you work in a dysfunctional company.
- AdrianB1 1y ago
  I saw this in many places, so I read that original statement like a complaint about a widespread problem, not an exception in one company. Sometimes is not even about a PR, it is about an entire project. I always do reviews (design and code, separate stages) for projects where code is almost complete when people come for design reviews and by the time we get to code reviews it is usually too late to fix problems other than showstoppers. I worked in small companies, huge companies (over 100k employees), some are better, most are bad, in my experience. YMMV, of course.
sdeframond 1y ago
I share your feelings. Regarding design reviews, we used to have them at my current job. However we stopped doing both formal design documents and design reviews in favor of prototyping and iterative design. The issue with the design phase is that we often failed to account for some important details. We spent considerable time discussing things and, when implementing, realized that we omitted some important detail or insight. But since we already invested that much time in the design phase, it was tempting to take shortcuts. What's more, design reviews were not conducted by the whole team, since it would be counter-productive to have 10-more people in the same room. So we'd still discover things during code reviews. And then not everyone is good at/motivated to producing good design documents. In the end, I believe that any development team above 5 people is bound to encounter these kinds of inefficiencies. The ideal setup is to put 5 people in the same room with the PO and close to a few key users.
- benji-york 1y ago
  > The ideal setup is to put 5 people in the same room with the PO and close to a few key users. (I suspect you are aware, but just in case this is new to you.) This is essentially the core of Extreme Programming.
  - sdeframond 1y ago
    What team size does XP recommends, if any ? It seems like the standard around me is between 8 to 12 people. This is too many in my opinion. I believe this is because management is unknowingly aiming for the biggest team the does not completely halts instead of seeking a team that delivers the most bang for the buck.
    - benji-york 1y ago
      I don't know of a canonical XP team size, but "small" is the normal recommendation. 8-12 would seem larger than small to me. Personally, 8 is the largest I would have a team. At that point you should consider breaking it into two teams of 4 (even if those teams periodically recombine from the original set of 8 people).
ghosty141 1y ago
I work in a small team where we are essentially 4-6 core developers. When I develop a feature I usually talk about it with my coworkers once I made a rough draft in my head how I'd approach it. They do the same so our code reviews are mostly only the minor code smells etc. but we usually decide on the architecture together (2-3 people usually). I find this to be one of the most important things in our team. Once people don't agree on code it all kinda goes downhill with nobody wanting to interact with code they didn't write for various reasons. In bigger orgs I believe it's still doable this way as long as responsibilities are shared properly and it's not just 4 guys who know it all and 40 others depend on them.
- sigmaml 1y ago
  I strongly second this. In my own experience of about 30 years, I have seen this method to work almost always.
flir 1y ago
You can get the same thing if you do user interface testing after you've built the thing. A design system can help there - at the very least, the change can feed back into the next revision of the playbook. Even if you can't fix it this time, hopefully you've taught someone a better pattern. The direction of travel should still be positive. On personal projects I've used architectural decision records, but I've never tried them with a team.
ruslan_sure 1y ago
This can be used in any process where the result is only judged at the end. The solution here may be to add a midterm check. I think this is what you mean by a "design review." In my experience, there are some rules that need to be followed for it to work. - Keep the number of stakeholders involved in all decisions, including PR, as small as possible. - Everyone involved should take part in this check. That way, no one will be surprised by the results. - This check should have been documented, like in the ticket. This can be used in any process where the result is only judged at the end. The solution here may be to add a midterm check. I think this is what you mean by a "design review." In my experience, there are some rules that need to be followed for it to work. We should keep the number of stakeholders involved in all decisions, including PR, as small as possible. Everyone involved should take part in this mid-term check. That way, no one will be surprised by the results. This check should have been documented, like in the ticket. When and how to do this check and how to handle disagreements depend on the task, culture, and personalities.
- ruslan_sure 1y ago
  We should do something similar with AI-coding. If you don't have a documented mid-term check, vibe-coded PR might not be what you expected.
guytv 1y ago
With AI based coding (no, i won't use "Vibe coding", thank you) this workflow improves a lot. Instead of jumping straight into code, I have my engineers write a Notion doc that describes what needs to be built. Think of it like an LLD, but really it’s a prompt for Claude-code. This forces them to think through the problem at a low level, and they share the doc with me before sending it to Claude — so I get to review early in the process. Once we finalize this "LLD" or "low-level-prompt", they hand it to Claude. The next time I see the work is in a GitHub PR. At that point, we rarely have to throw everything away and start from scratch.

by 3036e4 1y ago

epolanski 1y ago

> and that isn't something I ever encountered in the wild (in any formal sense) Because in the software engineering world there is very little engineering involved. That being said, I also think that the industry is unwilling to accept the slowliness of the proper engineering process for various reasons, including non criticality of most software and the possibility to amend bugs and errors on the fly. Other engineering fields enjoy no such luxuries, the bridge either holds the train or it doesn't, you either nailed the manufacturing plant or there's little room for fixing, the plane's engine either works or not Different stakes and patching opportunities lend to different practices.

ozim 1y ago
It still is engineering you only mistake design phase. Writing code is the design phase. You don’t need design phase for doing design. Will drop link to relevant video later.
- ozim 1y ago
  This is the talk on real software engineering: https://www.youtube.com/watch?v=RhdlBHHimeM https://www.youtube.com/watch?v=RhdlBHHimeM
- jmholla 1y ago
  I also read this series of blog posts recently where the author, Hillel Wayne, talked to several "traditional" engineers that had made the switch to software. He came to a similar conclusion and while I was previously on the fence of how much of what software developers do could be considered engineering, it convinced me that software engineer is a valid title and that what we do is engineering. First post here: https://www.hillelwayne.com/post/are-we-really-engineers/ https://www.hillelwayne.com/post/are-we-really-engineers/
  - 3036e4 1y ago
    I was an undergraduate (computer) engineer student, but like many of my friends at that time (dot-com boom) I did not graduate since it was too tempting to get a job and get well paid instead. However many, probably half, that I work with, and most that I worked with overall for the last 25+ years (since after I dropped out) have an engineering degree. Especially the younger ones, since this century it has been more focus on getting a degree and fewer seems to drop out early to get a job like many of us did in my days. So when American employers insist on giving me titles like "software engineer" I cringe. It's embarrassing really, since I am surrounded by so many that have a real engineering degree, and I don't. It's like if I dropped out of medical school and then people started calling me "doctor" even if I wasn't one, legally. It would be amazing if we could find a better word so that non-engineers like me are not confused with the legally real engineers.

teiferer 1y ago

Seems like your organization is lacking structure and communication. Where I work, the structure is such that most parts of the codebase have a team that is responsible for it and does the vast majority of changes there. If any "outsider" plans a change, they come talk to the team and coordinates. And we also have strong intra-team communication. It's clear who is working on what and we have design reviews to agree on the "how" within the team. It's rare that what you describe happens. 95% of the code reviews I do are without comments or only with minor suggestions for improvement. Mainly because we have developed a culture of talking to each other about major things beforehand and writing the code is really just the last step in the process. We also have developed a somewhat consistent style within the teams. Not necessarily across the teams, but that's ok. TL;DR: It's certainly possible to do things better that what you are experiencing. It's a matter of structure, communication and culture.

DixieDev 1y ago

Where I work we tend to write RFCs for fundamental design decisions. Deciding what counts as a "fundamental design decision" is sometimes self-moderated in the moment but we also account for it when making long term plans. For example when initially creating epics in Jira we might find it hard to flesh out as we don't really know how we're going to approach it, so we just start it off with a task to write an RFC. These can be written either for just our team or for the eyes of all other software teams. In the latter case we put these forward as RFCs for discussion in a fortnightly meeting, which is announced well in advance so people can read them, leave comments beforehand, and only need to attend the meeting if there's an RFC of interest to them up for discussion. This has gone pretty well for us! It can feel like a pain to write some of these, and at times I think we overuse them somewhat, but I much prefer our approach to any other place I've worked where we didn't have any sort of collaborative design process in place at all.

unkulunkulu 1y ago
I view this process like this: code review is a communication tool: you can discuss concrete decisions vs hand waving and explaining in the conceptual space, which of course has its place, but is limited. But writing the whole working code just to discuss some APIs is too much and will require extra work to change if problems are surfaced on review. So a design document is something in the middle: it should draw a line where the picture of the planned change is as clear as possible and can be communicated with shareholders. Other possible middle grounds include PRs that don’t pass all tests or that don’t even build at all. You just have to choose the most appropriate sequence of communication tools to come to agreements in the team and come to a point where the team is on the same page on all the decisions and how the final picture looks.

skybrian 1y ago

One way to fix it: pair programming. You're getting feedback in real time as you write the code. Unfortunately, the conditions where it works well can be difficult to set up. You need people who are into it and have similar schedules. And you don't want two people waiting for tests to run.

grilledcheez 1y ago
It's crazy that we go out of our way to design processes (code review, design review) to avoid actually just... working together? And then we design organizations that optimize for those processes instead of optimizing for collaboration.
guytv 1y ago
Anyone actually done long-term pair programming and lived to tell the tale? Is it real, or just a utopian fantasy?
- skybrian 1y ago
  I did it at a startup for a few months. The startup failed, but I think it was more of a business failure. Pivotal Labs was a contracting firm that did it for years. They aren’t around anymore, but they had a good run: https://en.m.wikipedia.org/wiki/Pivotal_Labs https://en.m.wikipedia.org/wiki/Pivotal_Labs
- jacques_chester 1y ago
  It's not for everyone. Some people have excellent reasons why it isn't workable for them. Others have had terrible experiences. It takes a great deal of practice to be a good pair and, if you don't start by working with an experienced pair, your memories of pairing are unlikely to be fond. However. I paired full-time, all day, at Pivotal, for 5 years. It was incredible. Truly amazing. The only time in my career when I really thrived. I miss it badly.

Ozzie_osman 1y ago

> Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? You don't need to. I've seen this generally work with some mix of the following: 1. Try to decouple systems so that it's less likely for someone in a part of the org to make changes that negatively impact someone in a more distant part of the org. 2. Some design review process: can be formal "you will write a design doc, it will be reviewed and formally approved in a design committee" if you care more about integrity and less about speed, or can be "write a quick RFC document and share it to the relevant team(s)". 3. Some group of people that have broad context on the system/code-base (usually more senior or tenured engineers). Again, can be formal: "here is the design review committee" or less formal: "run it by these set of folks who know there stuff". If done well, I'd say you can get pretty broad coverage from a group like this. Definitely not "everyone in the entire company". That group can also redirect or pull others in. 4. Accept that the process will be a bit lossy. Not just because you may miss a reviewer, but also, because sometimes once you start implementing the reality of implementation is different than what people expect. You can design the process for this by encouraging POC or draft implementations or spikes, and set expectations that not all code is expected to make it into production (any creative process includes drafts, rewrites, etc that may not be part of the final form, but help explore the best final form). I've basically seen this work pretty well at company sizes from 5 engineers all the way up to thousands.

motorest 1y ago

> It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. That is a problem with your organization, not with Git or any version control system. PRs are orthogonal to it. If you drop by a PR without being aware of the ticket that made the PR happen and the whole discussion and decision process that led to the creation of said tickets, you are out of the loop. Your complain is like a book publisher complaining that the printing process is flawed because seeing the printed book coming out of the production line is the only time when all stakeholders really get involved. Only if you work in a dysfunctional company.

AdrianB1 1y ago
I saw this in many places, so I read that original statement like a complaint about a widespread problem, not an exception in one company. Sometimes is not even about a PR, it is about an entire project. I always do reviews (design and code, separate stages) for projects where code is almost complete when people come for design reviews and by the time we get to code reviews it is usually too late to fix problems other than showstoppers. I worked in small companies, huge companies (over 100k employees), some are better, most are bad, in my experience. YMMV, of course.

sdeframond 1y ago

I share your feelings. Regarding design reviews, we used to have them at my current job. However we stopped doing both formal design documents and design reviews in favor of prototyping and iterative design. The issue with the design phase is that we often failed to account for some important details. We spent considerable time discussing things and, when implementing, realized that we omitted some important detail or insight. But since we already invested that much time in the design phase, it was tempting to take shortcuts. What's more, design reviews were not conducted by the whole team, since it would be counter-productive to have 10-more people in the same room. So we'd still discover things during code reviews. And then not everyone is good at/motivated to producing good design documents. In the end, I believe that any development team above 5 people is bound to encounter these kinds of inefficiencies. The ideal setup is to put 5 people in the same room with the PO and close to a few key users.

benji-york 1y ago
> The ideal setup is to put 5 people in the same room with the PO and close to a few key users. (I suspect you are aware, but just in case this is new to you.) This is essentially the core of Extreme Programming.
- sdeframond 1y ago
  What team size does XP recommends, if any ? It seems like the standard around me is between 8 to 12 people. This is too many in my opinion. I believe this is because management is unknowingly aiming for the biggest team the does not completely halts instead of seeking a team that delivers the most bang for the buck.
  - benji-york 1y ago
    I don't know of a canonical XP team size, but "small" is the normal recommendation. 8-12 would seem larger than small to me. Personally, 8 is the largest I would have a team. At that point you should consider breaking it into two teams of 4 (even if those teams periodically recombine from the original set of 8 people).

ghosty141 1y ago

I work in a small team where we are essentially 4-6 core developers. When I develop a feature I usually talk about it with my coworkers once I made a rough draft in my head how I'd approach it. They do the same so our code reviews are mostly only the minor code smells etc. but we usually decide on the architecture together (2-3 people usually). I find this to be one of the most important things in our team. Once people don't agree on code it all kinda goes downhill with nobody wanting to interact with code they didn't write for various reasons. In bigger orgs I believe it's still doable this way as long as responsibilities are shared properly and it's not just 4 guys who know it all and 40 others depend on them.

sigmaml 1y ago
I strongly second this. In my own experience of about 30 years, I have seen this method to work almost always.

flir 1y ago

You can get the same thing if you do user interface testing after you've built the thing. A design system can help there - at the very least, the change can feed back into the next revision of the playbook. Even if you can't fix it this time, hopefully you've taught someone a better pattern. The direction of travel should still be positive. On personal projects I've used architectural decision records, but I've never tried them with a team.

ruslan_sure 1y ago

This can be used in any process where the result is only judged at the end. The solution here may be to add a midterm check. I think this is what you mean by a "design review." In my experience, there are some rules that need to be followed for it to work. - Keep the number of stakeholders involved in all decisions, including PR, as small as possible. - Everyone involved should take part in this check. That way, no one will be surprised by the results. - This check should have been documented, like in the ticket. This can be used in any process where the result is only judged at the end. The solution here may be to add a midterm check. I think this is what you mean by a "design review." In my experience, there are some rules that need to be followed for it to work. We should keep the number of stakeholders involved in all decisions, including PR, as small as possible. Everyone involved should take part in this mid-term check. That way, no one will be surprised by the results. This check should have been documented, like in the ticket. When and how to do this check and how to handle disagreements depend on the task, culture, and personalities.

ruslan_sure 1y ago
We should do something similar with AI-coding. If you don't have a documented mid-term check, vibe-coded PR might not be what you expected.

guytv 1y ago

With AI based coding (no, i won't use "Vibe coding", thank you) this workflow improves a lot. Instead of jumping straight into code, I have my engineers write a Notion doc that describes what needs to be built. Think of it like an LLD, but really it’s a prompt for Claude-code. This forces them to think through the problem at a low level, and they share the doc with me before sending it to Claude — so I get to review early in the process. Once we finalize this "LLD" or "low-level-prompt", they hand it to Claude. The next time I see the work is in a GitHub PR. At that point, we rarely have to throw everything away and start from scratch.