6 ms·

Where are the junior devs while their code is being reviewed? I'm not a software developer, but I'd be loath to review someone's work unless they have enough sk

by analog31 10mo ago

Where are the junior devs while their code is being reviewed? I'm not a software developer, but I'd be loath to review someone's work unless they have enough skin in the game to be present for the review.

stuaxo 10mo ago
Git PRs work on async model for reviews.
- DrewADesign 10mo ago
  And even then, in my experience, they work more like support tickets than business email, for which there are loose norms for response time, etc. Unless there’s a specific reason it needs to be urgently handled, people will prioritize other tasks.
tyrust 10mo ago
Code review is rarely done live. It's usually asynchronous, giving the reviewer plenty of time to read, digest, and give considered feedback on the changes. Perhaps a spicy patch would involve some kind of meeting. Or maybe in a mentor/mentee situation where you'd want high-bandwidth communication.
- jopsen 10mo ago
  Doing only IRL code reviews would certainly improve quality in some projects :) It's probably also fairly expensive to do.
  - comprev 10mo ago
    Pair programming? That is realtime code review by another human
  - colinb 10mo ago
    Fagan inspection has entered the room
  - jghn 10mo ago
    Am old enough that this was status quo for part of my career, and have also been in some groups that did this as a rejection of modern code review techniques. There are pros & cons to both sides. As you point out it's quite expensive in terms of time to do the in person style. Getting several people together is a big hassle. I've found that the code reviews themselves, and what people get out of them, are wildly different though. In person code reviews have been much more holistic in my experience, sometimes bordering on bigger picture planning. And much better as a learning tool for other people involved. Whereas the diff style online code review tends to be more focused on the immediate concerns. There's not a right or wrong answer between those tradeoffs, but people need to realize they're not the same thing.
  - stephen_cagle 10mo ago
    And yet... is it? Realtime means real discussion, and opportunity to align ever so slightly on a common standard (which we should write down!), and an opportunity to share tacit knowledge. It also increases the coverage area of code that each developer is at least somewhat familiar with. On a side note, I would love if the default was for these code reviews to be recorded. That way 2 years later when I am asked to modify some module that no one has touched in that span, I could at least watch the code review and gleem something about how/why this was architect-ed the way it was.
    - lokar 10mo ago
      IMO, a lot of what I think you are getting at should be worked out in design before work starts.
  - Ekaros 10mo ago
    I would guess that 3 part code review would actually be most effective. Likely even save on costs. First part is walkthrough on call, next independent review and comments. Then per need an other call over fixes or discussion. Probably spend more time on it, but would share the understanding and alignment.
- throwaway314155 10mo ago
  My first job did IRL code reviews with at least two senior devs in the loop. It was both devastating and extremely helpful.
  - SoftTalker 10mo ago
    Yeah when we first started, "code review" was a weekly meeting of pretty much the entire dev team (maybe 10 people). Not all commits were reviewed, it was random and the developer would be notified a couple of days in advance that his code was chosen for review so that he could prepare to demo and defend it.
    - necovek 10mo ago
      Wow, that's a very arbitrary practice: do you remember roughly when was that? I was in a team in 2006 where we did the regular, 2-approve-code-reviews-per-change-proposal (along with fully integrated CI/CD, some of it through signed email but not full diffs like Linux patchsets, but only "commands" what branch to merge where).
      - SoftTalker 10mo ago
        Around that time frame. We had CI and if you broke the build or tests failed it was your job to drop anything else you were doing and fix it. Nothing reached the review stage unless it could build and pass unit tests.
        necovek 10mo ago
        Right, we already had both: pre-review build & test runs, and pre-merge CI (this actually ran on a temp, merged branch).
      - marwamc 10mo ago
        This was still practice at $BIG_FINANCE in the couple of years just before covid, although by that point such team reviews were reducing in importance and prominence.
rootusrootus 10mo ago
As someone else mentioned, the process is async. But I achieve a similar effect by requiring my team to review their own PRs before they expect a senior developer to review them and approve for merging. That solves some of the problem with people thinking it's okay to fire off a huge AI slop PR and make it the reviewer's responsibility to see how much the LLM hallucinated. No, you have to look at yourself first, because it's YOUR code no matter what tool you used to help write it.
- unbalancedevh 10mo ago
  > requiring my team to review their own PRs before they expect a senior developer to review them I'm having a hard time imagining the alternative. Do junior developers not take any pride in their work? I want to be sure my code works before I submit it for review. It's embarrassing to me if it fails basic requirements. And as a reviewer, what I want to see more than anything is how the developer assessed that their code works. I don't want to dig into the code unless I need to -- show me the validation and results, and convince me why I should approve it. I've seen plenty of examples of developers who don't know how to effectively validate their work, or document the validation. But that's different than no validation effort at all.
  - jjmarr 10mo ago
    Many are just doing SWE for the money. Their goal is to pass the hot potato to someone else, so they can say in the standup "oh I'm waiting on review" making it not their problem.
  - rootusrootus 10mo ago
    > Do junior developers not take any pride in their work? Yes. I have lost count of the number of PRs that have come to me where the developer added random blank lines and deleted others from code that was not even in the file they were supposed to be working in. I'm with you -- I review my own PRs just to make sure I didn't inadvertently include something that would make me look sloppy. I smoke test it, I write comments explaining the rationale, etc. But one of my core personality traits (mostly causing me pain, but useful in this instance) is how much I loathe being wrong, especially for silly reasons. Some people are very comfortable with just throwing stuff at the wall to see if it'll stick.
groby_b 10mo ago
I think we've moved on from the times where you brought a printout to the change control board to talk it through.
- SoftTalker 10mo ago
  But are we better off?
ok_dad 10mo ago
A senior dev should be mentoring and talking to a junior dev about a task well before it hits the review stage. You should discuss each task with them on a high level before assigning it, so they understand the task and its requirements first, then the review is more of a formality because you were involved at each step.
- marwamc 10mo ago
  Also communal RFCs, RFPs, Roadmapping, Architecture/Design Proposals, Design Docs and/or Reviews help socialize/diffuse org standards and expectations. I found these help ground the mentorship and discussions between junior-senior devs. And so even for the enterprising aka proactive junior devs who might start working on something in advance of plans/roadmaps, by the time they present that work for review, if the work followed org architectural and design patterns, the review and acceptance process flows smoothly. In my juinior days I was taught: if the org doesn't have a design or architectural SOP for the thing you're doing, find a couple of respectable RFCs from the internet, pick the three you like, and implement one. It's so much easier to stand on the shoulders of giants than to try and be the giant yourself.

stuaxo 10mo ago

Git PRs work on async model for reviews.

DrewADesign 10mo ago
And even then, in my experience, they work more like support tickets than business email, for which there are loose norms for response time, etc. Unless there’s a specific reason it needs to be urgently handled, people will prioritize other tasks.

tyrust 10mo ago

Code review is rarely done live. It's usually asynchronous, giving the reviewer plenty of time to read, digest, and give considered feedback on the changes. Perhaps a spicy patch would involve some kind of meeting. Or maybe in a mentor/mentee situation where you'd want high-bandwidth communication.

jopsen 10mo ago
Doing only IRL code reviews would certainly improve quality in some projects :) It's probably also fairly expensive to do.
- comprev 10mo ago
  Pair programming? That is realtime code review by another human
- colinb 10mo ago
  Fagan inspection has entered the room
- jghn 10mo ago
  Am old enough that this was status quo for part of my career, and have also been in some groups that did this as a rejection of modern code review techniques. There are pros & cons to both sides. As you point out it's quite expensive in terms of time to do the in person style. Getting several people together is a big hassle. I've found that the code reviews themselves, and what people get out of them, are wildly different though. In person code reviews have been much more holistic in my experience, sometimes bordering on bigger picture planning. And much better as a learning tool for other people involved. Whereas the diff style online code review tends to be more focused on the immediate concerns. There's not a right or wrong answer between those tradeoffs, but people need to realize they're not the same thing.
- stephen_cagle 10mo ago
  And yet... is it? Realtime means real discussion, and opportunity to align ever so slightly on a common standard (which we should write down!), and an opportunity to share tacit knowledge. It also increases the coverage area of code that each developer is at least somewhat familiar with. On a side note, I would love if the default was for these code reviews to be recorded. That way 2 years later when I am asked to modify some module that no one has touched in that span, I could at least watch the code review and gleem something about how/why this was architect-ed the way it was.
  - lokar 10mo ago
    IMO, a lot of what I think you are getting at should be worked out in design before work starts.
- Ekaros 10mo ago
  I would guess that 3 part code review would actually be most effective. Likely even save on costs. First part is walkthrough on call, next independent review and comments. Then per need an other call over fixes or discussion. Probably spend more time on it, but would share the understanding and alignment.
throwaway314155 10mo ago
My first job did IRL code reviews with at least two senior devs in the loop. It was both devastating and extremely helpful.
- SoftTalker 10mo ago
  Yeah when we first started, "code review" was a weekly meeting of pretty much the entire dev team (maybe 10 people). Not all commits were reviewed, it was random and the developer would be notified a couple of days in advance that his code was chosen for review so that he could prepare to demo and defend it.
  - necovek 10mo ago
    Wow, that's a very arbitrary practice: do you remember roughly when was that? I was in a team in 2006 where we did the regular, 2-approve-code-reviews-per-change-proposal (along with fully integrated CI/CD, some of it through signed email but not full diffs like Linux patchsets, but only "commands" what branch to merge where).
    - SoftTalker 10mo ago
      Around that time frame. We had CI and if you broke the build or tests failed it was your job to drop anything else you were doing and fix it. Nothing reached the review stage unless it could build and pass unit tests.
      - necovek 10mo ago
        Right, we already had both: pre-review build & test runs, and pre-merge CI (this actually ran on a temp, merged branch).
    - marwamc 10mo ago
      This was still practice at $BIG_FINANCE in the couple of years just before covid, although by that point such team reviews were reducing in importance and prominence.

rootusrootus 10mo ago

As someone else mentioned, the process is async. But I achieve a similar effect by requiring my team to review their own PRs before they expect a senior developer to review them and approve for merging. That solves some of the problem with people thinking it's okay to fire off a huge AI slop PR and make it the reviewer's responsibility to see how much the LLM hallucinated. No, you have to look at yourself first, because it's YOUR code no matter what tool you used to help write it.

unbalancedevh 10mo ago
> requiring my team to review their own PRs before they expect a senior developer to review them I'm having a hard time imagining the alternative. Do junior developers not take any pride in their work? I want to be sure my code works before I submit it for review. It's embarrassing to me if it fails basic requirements. And as a reviewer, what I want to see more than anything is how the developer assessed that their code works. I don't want to dig into the code unless I need to -- show me the validation and results, and convince me why I should approve it. I've seen plenty of examples of developers who don't know how to effectively validate their work, or document the validation. But that's different than no validation effort at all.
- jjmarr 10mo ago
  Many are just doing SWE for the money. Their goal is to pass the hot potato to someone else, so they can say in the standup "oh I'm waiting on review" making it not their problem.
- rootusrootus 10mo ago
  > Do junior developers not take any pride in their work? Yes. I have lost count of the number of PRs that have come to me where the developer added random blank lines and deleted others from code that was not even in the file they were supposed to be working in. I'm with you -- I review my own PRs just to make sure I didn't inadvertently include something that would make me look sloppy. I smoke test it, I write comments explaining the rationale, etc. But one of my core personality traits (mostly causing me pain, but useful in this instance) is how much I loathe being wrong, especially for silly reasons. Some people are very comfortable with just throwing stuff at the wall to see if it'll stick.

groby_b 10mo ago

I think we've moved on from the times where you brought a printout to the change control board to talk it through.

SoftTalker 10mo ago
But are we better off?

ok_dad 10mo ago

A senior dev should be mentoring and talking to a junior dev about a task well before it hits the review stage. You should discuss each task with them on a high level before assigning it, so they understand the task and its requirements first, then the review is more of a formality because you were involved at each step.

marwamc 10mo ago
Also communal RFCs, RFPs, Roadmapping, Architecture/Design Proposals, Design Docs and/or Reviews help socialize/diffuse org standards and expectations. I found these help ground the mentorship and discussions between junior-senior devs. And so even for the enterprising aka proactive junior devs who might start working on something in advance of plans/roadmaps, by the time they present that work for review, if the work followed org architectural and design patterns, the review and acceptance process flows smoothly. In my juinior days I was taught: if the org doesn't have a design or architectural SOP for the thing you're doing, find a couple of respectable RFCs from the internet, pick the three you like, and implement one. It's so much easier to stand on the shoulders of giants than to try and be the giant yourself.