4 ms·

Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your revi

by ar_lan 3y ago

Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your review for eyes to look at, you'll come to a point where your reviews should only go through 0-2 revisions before submission is ready. Personally, if I see a review that is >10 files that isn't explicitly a "Refactor" review, or I've been prepped ahead of time, it's probably going to take a long time to get the review out, because there are so many things to iterate on. I also have to block out a lot of time to even do the singular review, because it is so long and there is so much cognitive load to carry with it. Smaller reviews are almost always better. If a review really can't be "completed" in a single PR, then I've also suggested 1/x reviews where bones are placed but gated under an FSS or something similar. This prevents code that shouldn't be run from being run until the whole feature (even if small) is complete, and lets the reviews focus on independent parts. -- Essentially, I'm saying I disagree with you placing the "error" on the senior developers in this scenario. Burdening someone with insanely large reviews is the err of the submitter.

BlargMcLarg 3y ago
Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es). The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.
- ar_lan 3y ago
  I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard. Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path. A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3. -- > Merging side branch into side branch is a valid strategy Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.
chefandy 3y ago
I agree that the coding expectations are fine-- and I'm not saying you do this-- but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure. In any field, someone consistently acting like a know-it-all is indulging their own emotional shortcomings under the guise of enforcing good practices. Firstly, if this is a brand new junior and they didn't have the guidance to avoid this problem in the first place, that's on the senior developer(s). These are on-the-job-learned skills, and the reason junior developers make less money is because they need guidance from seniors figuring out how all of that stuff works. Secondly, if they did have explicit guidance, have been advised to tighten things up a few times, yet can't swing it, the senior developer needs to be a senior developer and empathically help them work through whatever strategic block is causing the problem. Finally, if the junior has been told many times and not cleaned up their practices, the senior developer isn't doing them any favors by playing the role of rankled, imperious elder-- the person is likely not cut out for the role, or needs to spend more time learning, maybe as an intern. It needs to be addressed with their manager so the right person can fill that position. If senior developers don't want to do that then they should work some place that doesn't hire juniors.
- ar_lan 3y ago
  > but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure Completely agree. If I get a chance, I almost always try to have a conversation (Zoom, in person) instead of writing large walls of text. It's definitely discouraging to, really anybody, to see your review get slammed by someone. Usually, if I see a common pattern or something is just wholly wrong, I try to whiteboard it out with them instead. I hope I've always come across nicer/a good mentor from this. I'm sure someone has disagreed :) -- All in all I completely agree. Senior devs + managers need to set the stage, expectations, and provide the necessary mentorship/utilities needed to accomplish. Also, a pet peeve of mine - one of my first teams I was on, a dev always commented on style issues. It got to the point where numerous junior devs complained and finally some other engineer stepped in and said "I don't disagree with your style comments, but you'd save yourself the headache if you just wrote a linter to catch that automatically." It's a pretty clear example in my mind of someone who finds self-importance in their voice being shown on each code review, when a simple utility would save everyone the headache.

by ar_lan 3y ago

BlargMcLarg 3y ago

Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es). The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.

ar_lan 3y ago
I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard. Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path. A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3. -- > Merging side branch into side branch is a valid strategy Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.

chefandy 3y ago

I agree that the coding expectations are fine-- and I'm not saying you do this-- but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure. In any field, someone consistently acting like a know-it-all is indulging their own emotional shortcomings under the guise of enforcing good practices. Firstly, if this is a brand new junior and they didn't have the guidance to avoid this problem in the first place, that's on the senior developer(s). These are on-the-job-learned skills, and the reason junior developers make less money is because they need guidance from seniors figuring out how all of that stuff works. Secondly, if they did have explicit guidance, have been advised to tighten things up a few times, yet can't swing it, the senior developer needs to be a senior developer and empathically help them work through whatever strategic block is causing the problem. Finally, if the junior has been told many times and not cleaned up their practices, the senior developer isn't doing them any favors by playing the role of rankled, imperious elder-- the person is likely not cut out for the role, or needs to spend more time learning, maybe as an intern. It needs to be addressed with their manager so the right person can fill that position. If senior developers don't want to do that then they should work some place that doesn't hire juniors.

ar_lan 3y ago
> but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure Completely agree. If I get a chance, I almost always try to have a conversation (Zoom, in person) instead of writing large walls of text. It's definitely discouraging to, really anybody, to see your review get slammed by someone. Usually, if I see a common pattern or something is just wholly wrong, I try to whiteboard it out with them instead. I hope I've always come across nicer/a good mentor from this. I'm sure someone has disagreed :) -- All in all I completely agree. Senior devs + managers need to set the stage, expectations, and provide the necessary mentorship/utilities needed to accomplish. Also, a pet peeve of mine - one of my first teams I was on, a dev always commented on style issues. It got to the point where numerous junior devs complained and finally some other engineer stepped in and said "I don't disagree with your style comments, but you'd save yourself the headache if you just wrote a linter to catch that automatically." It's a pretty clear example in my mind of someone who finds self-importance in their voice being shown on each code review, when a simple utility would save everyone the headache.