My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship (<<< keyword) is maintainable, tested, and documented as far as any gotchas or hacks go. Ideally we want to improve the code base as well, especially when dealing with legacy code.

I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leave any comments or get any on my PRs for them since we are all on the same wavelength and for the most part know what we are doing.

A junior engineer is a… junior engineer. They are going to have a lot of comments telling them how to write better code and a good suggestion will include a code suggestion and links to read.

It’s how they will get better, and for bigger tickets or super juniors, I sometimes pair with them to go through their ticket together.

At a certain level you just know how to identify patterns and understand how to build good software. You read books like Clean Architecture or Designing Data Intensive Applications to level up.

Juniors tend to just write yolo shit code or add more shit to a legacy shitcode repo instead of trying to improve it. They’ll add to the mess instead of writing a new clean layer on top to which we can easily maintain and modify. But that’s why you have to show them.

The only thing that helps people get better is time, being humble, and being genuinely interested themselves in becoming better at their craft. I remember my junior engineer days, and seeing my PRs filled with comments. But if I didn’t have those I don’t think I’d have leveled up so fast.

That says nothing without context.

If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit someone's architectural tastes.

It makes no sense to criticize someone for focusing where it matters the most and actually deliver critical features without delay if the tradeoff is some legacy debt, and that by no mean implies they're junior.

One important skill of a senior is being able to keep their eyes on the prize and be smart about tradeoffs. Prioritizing subjective opinions over software architecture over delivering value is more in line with a junior way of thinking about software than not, and to me doesn't sound like a position to criticize others.

The problem is people don't want to expend the very minor extra amount of effort.

So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line.

After a while you have a giant festering pile of shit that you just keep making bigger and bigger and now people are talking about rewriting it.

In my experience you start actively losing velocity in this situation very quickly. If you're doing this when you still your have 3+ months on a project, you're likely making poor decisions.

I'm not saying you need to constantly gold plate your architecture. But it should be palatable.

The subjective part is what is palatable. But I don't know anyone who thinks steaming turdpiles are palatable.

The interesting thing is that this isn't a technical problem, it's a human problem. Most folks want to clock in, do their job and clock out, and going the extra mile to improve things isn't worth the effort.

I think its the same human tendency that leaves so much pee on the toilet floor - some humans will clean up their own, but almost none will clean up someone else's.

I suspect this is because it’s almost never rewarded. Either because nobody up the chain of command understands and values quality beyond “it’s working and making us money right now so it’s fine” and also because doing things right is tiring. Why should I spend energy on this and not have that energy when I’m off the clock if I’m not going to get more money/recognition/time off for it and I’ll probably get laid off or move on in a couple years anyway.

I don’t think anyone deliberately starts a job or task thinking they’ll do the absolute minimum. People generally want to do their best but the system moderates this desire and that’s the result.

I don’t really have a solution, just pointing it out.

The solution is not to accept "it is what it is".

The time tax is paid by not delivering a feature. Architecture is supposed to be there to help you, not fail deadlines.

Also, it seems you completely failed to read or get what I wrote. My point was that if a PR delivers a feature without defects then your team is far better off if you accept the PR and refactor later when you can spare the time then it is if you throw a hissy fit and refuse to get the feature done if you don't get your way. You are paid to create value which is in delivering features and fixing bugs. Architecture is supposed to be there to help you.

Think about it for a second. If your architectural purity demands a hefty refactor, is that a good justification to postpone a bug fix or delivering a feature? No, it isn't.

> The problem is people don't want to expend the very minor extra amount of effort.

Except that most time it isn't minor.

You have to pass interfaces throughout a bunch of components, which require updating all sorts of tests, and result in a large code footprint.

> After a while you have a giant festering pile of shit (...)

That only happens if you're incompetent at your job and fail to do the most basic maintenance tasks in your code base.

Look at what you're trying to claim. It's supposedly easy and trouble-free to put together a PR that respects your personal notion of what the software architecture should be. Yet, the work to refactor code that breaks it is so insurmountable that you're no longer able to refactor it back to shape?

Which one is it then?

It looks like the FANG engineer you tried to badmouth had a firmer grasp on things and on what it matters the most than you do.

Most deadlines are artificial. If it's not artificial, yeah, you're going to have to sacrifice something for the sake of the real deadline. But you are sacrificing something, and it's not just subjective fluff.

> Also, it seems you completely failed to read or get what I wrote. My point was that if a PR delivers a feature without defects then your team is far better off if you accept the PR and refactor later when you can spare the time

You don't ever have spare time. When you finish a feature there's another waiting for you. At least, that's how it works literally everywhere I've worked. Working this way is just another form of over-promising and under-delivering.

> then it is if you throw a hissy fit and refuse to get the feature done if you don't get your way

Rejecting a change doesn't mean you're throwing a hissy fit.

> You are paid to create value which is in delivering features and fixing bugs. Architecture is supposed to be there to help you.

It does help you. In the medium to longer term. In the short term, literally any standard out there will slow you down - unit tests, CI/CD, shit - testing at all. It all slows you down. It doesn't mean we should just code like its 90s era PHP - the fact that we don't should be a strong signal about the value of this whole hypothesis you have going here.

> You have to pass interfaces throughout a bunch of components, which require updating all sorts of tests, and result in a large code footprint.

Funny. Your tests are slowing you down, why don't you just comment them out to get your feature in?

> Look at what you're trying to claim. It's supposedly easy and trouble-free to put together a PR that respects your personal notion of what the software architecture should be.

It's not actually just personal. We have unit tests that enforce certain architectural rules.

> Yet, the work to refactor code that breaks it is so insurmountable that you're no longer able to refactor it back to shape?

If you don't do these things for the sake of feature velocity, as a senior software engineer, why should anyone else? If no one is, how large of a problem do you think you're going to get? And do you really think its just as easy to fix something after the fact?

Of course, if you don't know how something should be, that's one thing. But knowing how it should be and just skipping it altogether is quite another.

Why are you even refactoring for reasons you view as subjective? I don't do subjective refactorings. Despite what you claim, at the point of a change, much of this stuff is very not subjective. If you show any developer a function call before a feature was added, and what it looks like after its been hacked it in some fucked way, no one is going to say the fucked version is better.

> It looks like the FANG engineer you tried to badmouth had a firmer grasp on things and on what it matters the most than you do.

Well maybe you're just so smart its trivial for you to navigate this stuff. If so, continue by all means. Until then, us mere mortals will have to adopt standards so our codebases don't outpace our ability to understand it.

Yes. That’s why it’s an important skill for an engineer to know when to make that tradeoff.

Sounds like you’re agreeing with the grandparent comment?

I get it, you've been burned out by Enterprise(tm) code. Going to the very edge of the other side isn't any better

I don't. I like to feel productive instead of having to do laundry before getting to the actual work because someone just went ahead without any regard for quality control

>YAGNI + KISS > SOLID

Agreed

Monster architectures are monster architectures, and yet going from Enterprise(tm) architecture to chaos architecture so you can't know anything about the system without carefully going over hundreds of lines of code isn't any better

Also depending on the codebase (and language to some degree), if someone senior writes a lot of overcomplex or abstract code that the rest of the team can't understand/maintain, that's just as much a problem as anything else.

For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works.

Any tips for giving feedback to senior people here? A lot of the complexity from abstraction is very hard to quantify and experienced people can have arguments that sound reasonable.

Perhaps a starting point for this discussion is to point out that the DRY principle is nice and all, but there is also WET. Premature abstractions are bad code, a liability and hinder development. It's always better to have two independent but mostly similar codepaths than a strategy pattern with two concrete implementations. If it's cleaner to copy/paste a method and do some tweaks, do that instead. It's not like you can't refactor it when additional use cases emerge, and if they don't emerge then the abstraction wasn't needed to begin with.

Plenty of people believe that being senior is being clever with complex stuff. It isn't. Being senior is to know you don't need complex stuff, and be able to keep as simple as possible. Abstractions go against that.

Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.

* Interfaces(particularly in java) that have 1 implementation.

* Interfaces with many implementations but where each implementation is used in exactly 1 place.

* large inheritance hierarchies with generic type parameters. I'm sure there's a good use case for this but it's usually a pain.

* Writing "generic" code in an effort to make something re-usable when in reality the code has knowledge of every location it is used in and tightly couples all implementations

With an interface I know that only that method can be called. If a class is passed in i have no idea what methods going to be used. It also gives me a better feeling that the coder has in mind what they need a parameter to do without worrying that they are going to rely on a couple of othogonal methods to be called.

In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity in the first year used to be a rule, when bringing juniors onboard in the dark ages of computing. With 100% code reviews, that process seems to take no more than a couple of months -- maybe even as little as one month. The difference is dramatic.

And it's a fabulous way to transmit not only good culture, but also love of our craft.

I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback.

I’m looking for basics. Will it work? is the code clear? is it commented/documented? is the approach simple and maintainable?

Some submitters really need coaching on how to receive feedback, and how to plan their timelines to incorporate revisions.

Thirty years later, egoless programming is one of Robert Glass's favorite fallacies in 'Facts and Fallacies of Software Engineering.'

IOW, it is a hard problem!

The SE research community has been looking into this for awhile (e.g. Greiler, Bacchelli)

[0] 'The Psychology of Computer Programming' (1971), Gerald Weinberg

[1] 'Facts and Fallacies of Software Engineering', Robert L. Glass

If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated?

I aim for one or at most two rounds of review, because anything else is just depressing.

Designing features, architecture and learning should happen before someone even gets to the point of submitting a PR. Plus if it can be improved on in follow PRs then do that instead of blocking juniors with pedantics

Providing recommendations takes away autonomy from the person who wants to contribute, and it's easy to end up glossing over the real consequences and ending up with "well, I would have done it this way" nit picking reviews.

“Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!

"Do [X]"

Now I write:

"I see [problem Y], consider making [change X] to improve it"

If the reviewee agrees then the change is easy and straightforward to make, but if they're unconvinced then the phrasing invites a dialog.

Or if I think I see a bug, I'll phrase it like:

"I think there's a bug here, how does this method behave if foo is null and bar is the empty string? I think we'd throw a null pointer error. Recommend adding a test for that case"

Clear, actionable, refutable

That's wisdom, and a clear way to make everyone around you better.

I'd add that being humble should also play a role. We have tastes and insights and preferences, and it's not productive to block PRs because of subjective, non-critical aspects. A working CICD pipeline lowers the cost of pushing a change, this we can always revisit things. It's far more important to have a team that trusts each other and feels confident to push changes fast than it is to have gatekeepers whose role ends up being one of needlessly putting breaks on a team for no justifiable reason.

“I’m not sure if I understand the whole idea but could you explain what this method does?” misdirects by suggesting the reviewer thinks they need education, rather than that the reviewer thinks the code can be clearer.

Don't make teachers the punching bag for bad programmer code review. Programmers get paid 3-4x more, so if teachers can figure it out, then why can't programmers?

Meanwhile, teaching at code review means that student gets exercise, donit without guidance and then gets list of everything that was wrong with it. And then gets told that wherever his opinion preference differs from the reviewers one, he is wrong too.

No good teacher does that.

I mean going through engineering school and rigorous STEM degrees I can say that stuff is baked into the formula. You’re derided and dogged and gaslit from the onset.

Is it surprising these people graduate, become senior and perpetuate the mental unhealth?

No, instead you have your ego brutalized by spending half your youth (not to mention tens or even hundreds of thousands of dollars) getting an undergraduate and master's degree and teaching certification...only to receive poverty wages, pay for your own supplies, be abused by students and parents and administrators and HN commenters alike...

Also, varies by country: the second world has "pedagogical universities/colleges" which are focused on producing school-level teachers in every subject taught in the schools — maths, chemistry, CS, you name it... and usually they have the lowest requirements compared to any other universities, so people apply there for a "last resort higher education" so to speak.

And then some people are bright enough to do well there, and some are not quite, and after graduation the smarter ones generally manage to find a better job than to be a school teacher while the dumber ones, well, they apply to schools to teach. And they get employed because schools almost always lack teachers. Yay...

I imagine things are better in the first world.

For new people or junior people who have the right attitude, this is poor advice. Where else will people get habituated to the standards, preferred idioms and implementation quirks of the team/company/product/subsystem?

This is literally Dunning-Kruger, in effect.

Standards documentation, code labs, pair programming, instructor-led group trainings. I agree that code review is a necessary piece of the puzzle, but there are other places for engineers to acclimate too.

Perfection need not be the enemy of good enough. Usually, whether the documentation is out of date or not, _some_ documentation is better than none. I've rarely come across an instance where documentation was actively harmful -- not to say those cases don't exist on the margins.

> And I do not even know what 'code labs' even means.

Code labs are self-guided lessons, like a tutorial, about the systems and technologies that your company is built on. They're called code labs because producing code/artifacts as part of your learning is one of the goals. For example, your company may have a custom enterprise installation process that's built upon Ansible, Terraform, AWS tooling, and a little proprietary work sprinkled on top. Some code labs may take you step-by-step through how to add a new service; configure, migrate, and wire up databases; etc. such that you'd have working code at the end of the code lab and new knowledge around the company's best practices.

Similarly you can do this for the standard language at your company. Most companies accumulate some amount of proprietary helpers, libraries, or frameworks in their standard language. A code lab can walk you through how to write code in the way your company expects using the helpers, libraries, and frameworks that are common across your teams.

> My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.

I have over a decade of similar experience across startups and large companies in NYC, Seattle, and SV. In the last 5 companies I've been at, all of them have had some combination of the above.

In case the author is around, please consider removing the email capture popup. It not only interrupted my reading of the article before getting to the main point if it - it had an animation that literally startled me, and I immediately closed the site. I can't believe I got jump scared by an ad in an article but it was incredibly offensive.

Just tell me why you don’t like the function name instead of trying to take me on a little thinking quest for my little brain

For the contrived example about renaming a method, no I dont think you should ask why they named it that way.

It’s similar to the idea of “being offended” which is pretty common these days. It’s been said that offense is taken not given. So the reader is usually the problem. Ignoring of course the blatantly offensive topics and communications, which do still exists. But in a work situation, you should always approach things with an assumption the other person is trying to be constructive and make an effort to not be offended by something that initially offends you or was delivered in a terse manner. If a patterns presents itself, have a conversation and ask why it’s being delivered that way.

I've had people tell me they think its insane how some people can't handle code reviews and would go to their manager about it.

If I have a serious concern I will also message/talk to the person directly. We need to reach an agreement going forwards, rather than trying to "play the game" and slip code past each other's standards. Direct communication creates good relationships and honest intentions between developers.

MRs can be a good opportunity to publically ariculate design decisions and trade offs. In that sense, they are good for learning. Small, well-defined, succinct issues/MRs can be an invaluable form of documentation, helping you to reconcile bugs with expected behaviour months or even years down the line.

I think it's important to keep MRs fast. The more friction there is to merging work, the less likely developers are to break up and integrate their work in clean, intelligible, atomic commits.

I call this "reviewing for accent". Like, it probably doesn't matter if you use !!foo or Boolean(foo), we all know what they both mean.

On the other hand, if someone is loading images on a main thread of a UI app, teaching is a primary priority compared to getting the change merged. Obviously they are misunderstanding big parts of platform they are developing for and, until I get them up to speed, they will have a hard time being productive. So even if it takes an extra week, it's important that they learn the best practices and how to apply these in specific cases.

I am absolutely against mind games like asking vague questions and holding up work until the author gives me my preferred answer, neither of us have time for that.

I will say if I submit a change and my reviewer immediately opens a change to make minor edits I'm going to be annoyed.

Either it's important enough to bring up in review, or it's too minor to bother with.

The problem here is the mind games. If you think a method name is a problem, just say so. Don't hint and waste people's time and energy. Just say what you mean. (If you can't do that, you're in a terribly unhealthy work environment, and fixing that should be a top priority.)

For less trivial examples, oftentimes I think honestly asking "why did you do it this way?" Before making a suggestion is a good idea. Often we dont have the same context nor know the intention.

Emphasis on "honestly." If you just want them to do something differently then suggest that directly.

What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant?

If you are going to review at an eye-to-eye level, then you have to go in assuming best intentions. A method might seem poorly named, but if you're reviewing a peer then it is very likely that you just doesn't understand some context. I'd never jump in and assume a method is poorly named unless I understand why it was named that way: Chesterton's Fence.

That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?

But if it's the simple renaming example then by all means lead with the suggestion.

I do not know what happens. Do reviewer really cannot understand, or he is trying Socrates on me? Or maybe he have some undisclosed anxiety issues, and keep himself vague on purpose, because he is afraid to show me some inner processes of his mind? Maybe it is an imposter syndrome at work or something like that?

What should I do in such a situation? Should I answer his question in a direct way and to explain what is happening, or should I jump to a meta and try to guess why the reviewer asking me such question? I mean I need to go meta and to untangle all this mess, but I'm a programmer, not a psychologist. This is not a psychotherapy session. But I'm forced to imagine all the kinds of mind states to find those that may generate the question I've got, and then to find some reaction for me, that will do minimum damage regardless of the state and give me more information about the state. So my reaction will be a counter question. It is time spent for a small talk, with me carefully moving around to not trigger possible anxieties of the reviewer, while being unsure about his mental issues and not knowing what exactly I'm trying to avoid.

But the honest review when reviewer gives me his thoughts is much more informative. I need no more to guess what is going on. “I had a hard time grasping what the method does. What about changing the method name to openRequest() to make the methods objective clearer and improve code readability?” gives me all I need to proceed in a conversation. I know what reviewer wants from me (to change method name), and I know why he thinks so. I know my options: I can accept his change, reject it, or propose some third variant of the method name.

> I'd never jump in and assume a method is poorly named unless I understand why it was named that way: Chesterton's Fence.

Review process is not a process of editing wikipedia. My commit was not approved yet, I'm not a random anonymous person from Internet, and the reviewer also is not a random anonymous person. So Chesterton Fence is not applicable here.

If I think that my way to name method is better for some reason, I'll inform the reviewer on my opinion, and my reasons to think so. Moreover I can move the conversation one step further right now by addressing his concerns.

> This method name is a bit too vague, what is it trying to do?

it's especially galling if the original PR was working, reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "best practices".

Some of the comments here seem more like sabotaging junior developers career by drawing out how long it takes them to get through code reviews than teaching them anything.

"A feature not making it into a sprint" is your problem. You will get in trouble for that, it will make you look bad in front of your boss, etc. "Not following best practices" is not your problem. If the code turns into a mess, it's not you who will have to deal with the consequences, at least not you directly. As a manager you will demand that your senior developers fix that (while still shipping features, of course). After all, that's what expected of senior developers. So, the incentives for you and for your senior engineers are not aligned.

Of course I don't know if that's even remotely close to your reasoning. I don't know you or your project or your priorities, I have no idea what "best practices" we're talking about and how reasonable it is to follow or not follow them in a given situation. I might be totally wrong, you may have all the right reasons to be mad about those reviews. But the comment makes it sound like your senior engineers are idiots who block PRs for no reason, and that can't be good for your team.

Hmm, almost like this concept of “sprints” is unhelpful.

It’s rare that delaying a PR by 24 hours to get it right has any relevance to business outcomes, in fact it is often better for everyone in the long run.

But with artificial deadlines and fake metrics imposed by scrum, when this happens managers will often freak out because something didn’t “make it into the sprint,” or an engineer’s velocity dropped.

There's exceptions, clearly. And the process does work for some folks. I'm glad those people are happy. It has rarely worked for me, or for teams I've been on.

Ah, I see, you think this is a general problem I have. No, in general my senior engineers are excellent and all the current ones I work with are.

I've seen enough of the other, especially while contracting.

You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices.

I'd guess you also see half of the time your team spends on coding as some useless nitpicking you're letting them get away with by sheer generousity or bcause you don't have a choice...From the looks of it that can't be a healthy team dynamic

No, in general, I agree with code review comments my senior developers make. I'm talking about the difference between "best practices" and actual best practices or in the difference between "this works, but next time, a better architecture would be x, let's refactor next time we come back to this" and "I'm blowing up the sprint"

In my experience there is rarely ever a “next time” and “fix it later” becomes “fix it never”. It’s always cheapest to fix worst practices up front rather than letting them metastasize into a huge pile of technical debt later. It’s also rare to find anybody interested in or willing to go back and fix old stuff when priorities often involve chasing the next shiny thing.

Then the code that gets touched the most gets touched up the most. Code that doesn't ever get touched is fine being kinda crappy.

When I hear comments like this from managers it’s generally a sign that I need to start planning an exit strategy.

edit: Also, “my senior developers?” They’re human beings, not your property, dude

As a manager, the Junior shouldn’t be on the critical path solo. That’s just bad management.

hmm...this would sound so much better if you felt your team was pushing for "actual" best practices.

Perhaps it's all working well, but from your words there's so much underlying tension and your clarification adds further distanciation between you and your team.

I'd hate to be on either side of this to be honest.

There are better places, and ways, to teach junior developers than long, painful code review processes.

If I had to choose the best single way to grow as a software engineer, it would be through code reviews: both giving and receiving.

That said, "long, painful" code reviews is a red flag for your team. A code review should have one of four outcomes:

1. Lgtm.

2. Approved. Left some suggestions for your consideration.

3. Specific changes requested.

4. This whole approach needs to be re-evaluated, let's talk.

_None_ of those should be long or painful. It sounds like your team might be doing #4 in the PR itself when really that's an exceptional case that should make it to a usually-synchronous discussion.

I think we are just talking about different scales and different senior developers. Every code review is either teaching something, finding missed bugs, find better ways to do things that a senior developer makes full use of.

If not, your code review is just a preformative waste of time.

I'm talking about simple PRs, the kind I would assign to a junior developer, that are actually finished, getting held up while a "teachable moment" unfolds.

I get what you're saying. Delaying a milestone to have a long convo in a CR is not good prioritization. But as the technical manager it's on you to budget more time for junior devs to land their changes.

Like what? And dont tell me it’s some kind of lecture/lab-style group meeting for 1-2hrs

I'm not talking about that.

In my opinion, you are looking at the problem from the wrong angle. If code gets worse (non working) after a code review, the reviewer made some serious mistakes and/or was not guiding his colleague to the correct solution like it should be. Also if working code gets rejected because of bad practice, you should not be mad about it but be thankful that someone caught that before release. If your only measurement of code quality, is that something is working, I would consider that a dangerous practice.

Concretely, a heuristic I rely on is to understand whether the sub-par code being added "infects" the code base e.g. it changes an important interface, it adds a poorly designed interface other code will rely on etc. These are the places its important to put your foot down. Conversely, if the internals of a one-off function or class could be a little cleaner... eh, ship it.

Holy shit. I should have just written this.

I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS.

Why are you assuming I want them all to just be rubber stamps?

And what God Complex do you have to have that you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager just don't respect "quality" at all?

'It works - why don't we just merge it? Keep velocity high!'

A code review is exactly where it's worth spending time making sure: the code is maintainable, doesn't degrade the quality of the repo, and above all teaches the junior things they can use next time to do a better job faster.

Spending some time using a review as a teaching experience pays so many dividends later. People who don't touch the code don't understand that.

Of course, there's a level of 'good enough' a senior should be able to identify and approve. But the bar should be high.

Because code reviews are a minimum. This is like saying "Why are there so many people assuming I don't give a shit about driving safety, I put on my seatbelt!" Yeah, it's because you're driving rashly and putting others at risk. Just putting on a seatbelt isn't the be-all and end-all of the process.

If you want to know, the reason that people are assuming you don't give a shit about quality are your sprint-centric attitude, your putting the word 'senior engineer' in double quotes, and the talk about "well if the PR was working then it should ship".

This gives off an impression of someone who doesn't understand at a fundamental level that the process of code reviews and have the contribute to learning on the team is is what makes for quality in the long run, not just the mere fact of having them.

Of course, it's quite possible you are the reasonable party here and your senior engineers are curmudgeons. In that case, have you tried talking to them about it? What did they say? Your complaint here seems to indicate that this hasn't happened yet.

Given a senior developer, a junior developer, and a non-technical manager, in the context of a code review, the vast majority of the time you should absolutely listen to the senior developer.

If that's not true in your organization, then hiring, leveling, and leadership should all be questioned.

> you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager...

Having code reviews is an incredibly low bar. The very fact that you offer this as an evidence might be the reason people make those assumptions.

I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint.

Saying "this can't ship without significant rework" is something that needs to get escalated, not ground out in a code review.

But that is a task

When I was a junior engineer, nothing annoyed me more than engineers trying to 'teach' during code reviews and doing so in one of two ways:

1. Being vague and unhelpful under the guise of having me 'figure it out'. They would point to a spot and say 'This needs fixing, rework this', without saying how to rework it other than to figure it out. Eventually I would need to drag them into a call to get them to tell me how they personally wanted it fixed up. Often times the way they wanted it fixed was incorrect because they misunderstood the work or were just wrong.

2. Giving incredibly bad 'improvements' to the code. Things like 'change X loop style to Y loop style' where Y doesn't affect the readability or performance of the code, or things just for the sake of adding comments.

So nowadays when I do PRs for juniors and such I try to have direct, actionable comments with rationale and room for disagreement, or if something needs to be reworked I try to assess why they did it that way and do a quick call/discussion with them. Junior engineers are generally not dumb and too many seniors treat them as such.

It works in reverse too, when you’re the one being reviewed and you receive feedback you disagree with.

Instead of dying on the hill and delaying your merge, just implement the feedback and then argue. You can make your point while still accommodating your teammate and moving things along.

(Obviously, this practice shouldn’t apply to feedback which is truly bad)

Real world example: a company I consulted for a couple years ago had a lead architect known for nit-picking code issues with junior devs ad nauseam (to the point where potential hires were told to expect it and not join if they didn't think they could take it). Their production system was brought to their knees by a doubled for loop, literally just a duplicated line of code. This code copied messages from one place to another, but almost all of their testing used cardinality 1 so it didn't affect the tests. In production all it took was a couple cardinality 2 cases because there was one edge case flow that could route copied messages back through the loop and this caused cascading failures as 2 became 4, 4 became 16, etc. This error would have been obvious to anybody reviewing the code, even if they didn't really know much about it, but as that module was written by said architect no one else felt qualified to review it.

Code reviews are important, but I think I know the type of person you are talking about which takes these things to an extreme and ends up being dead weight. Often times, they're not as good as they think either, just being honest.

You need to rein in these people ASAP, honestly they are the worst kind of coworkers. Toxic positivity and wasting time/effort 99% of the time. We're paid for our skills, and one of those skills is being able to figure things out on our own or ask for help with pertinent questions. This hand holding crap to get eyes on you is just silly and something I really dislike working at big corp.

For instance if you're telling them about a behavior defined in a RFC, it can have better long lasting impact if they get the time to read and understand it on their own, than if you're over their shoulder shoving info at them.

Basically, live teaching requires you to be good at reading the other person's state and providing the relevant info in a digestible way, while they live code. It can work, but that's a high bar to pass.

However, if this is in a small team, then it's a management problem that needs to be solved by the people manager. Either the hiring process is not eliminating bad performers or people with bad attitudes, or team dynamics have deteriorated considerably.

From the senior's viewpoint, though, if they are going to be held solely responsible for janitorial work to avert juniors' substandard work blowing up down the road, or picking up the pieces when that does happen, then it makes perfect sense for the senior to put up roadblocks, junior dev's career be damned. Again, this is likely a management problem where people are not held accountable and made to maintain systems they come up with (tenure is so short, especially among junior people, that this is likely to be a systemic problem).

You sound like an absolutely insufferable person to work under.

Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.

While we're extrapolating, are you the type that makes junior developers rewrite their loops as list comprehensions or the type that makes them rewrite their list comprehensions as loops? Because I've seen both in code reviews.

I agree you should push back against this as a manager, but it can be hard to do so tactfully from my experiences. You either have to say, "no," or engage in protracted debates on subjective ideas around readability and maintainability.

Just review this thread ... it can be utterly painful. There are at least as many senior devs who don't like to take feedback on how to deliver feedback as junior devs who don't like to take feedback.

There is not going to be one answer for every situation. Let's say your startup runs out of money tomorrow, the feature must be demo'd today to raise more money, is this code getting merged or debated? If this software for a life support system the bar is set very differently. What's the cost of failures, what's the cost of future maintenance, etc. - all matters.

If the senior engineer has enough projects/years under his belt, good judgment, has seen various business outcomes, and can weigh this, then I would generally trust them as being closest to the decision point. If those are the senior engineers on your team I don't think code reviews and mentoring juniors is going to be a problem.

I squashed that.

One of them is right, because a codebase that consistently uses one or the other is better than a codebase that mixes the two. If the team hasn't made and communicated a decision then that sounds like a management failing.

(list comprehensions are the actual right thing to use, for the record, but that's far less important than having a standard and sticking to it)

It's similar to naming conventions - you wouldn't want to mix snake_case and camelCase in the same code - only more so, because it's a bigger leap from one to another. Things that are the same should look the same and things that are different should look different. If each loop in your program is represented differently, it'll be hard to understand.

> If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one you pick)

Juniors can learn anything but they can't learn everything. The more trivia they have to deal with, the longer it will take to learn the big things. It doesn't matter in the same way that, say, it doesn't matter whether you pick Rails/Ruby or Python/Django for your webapp; it's still a bad idea to do both at the same time.

This is the worst time to try to fix anything. The author has just finished (or thinks he finished) the work and any attempt to change anything by the reviewer is going to elicit resentment in most people. Try to tell the person the change cannot pass and you can make an enemy. Or you let the change in because you like the person. Either way, it is bad.

And all this for naught because in my experience the law of sunken cost comes into action and people will try to push it as much as possible unless it really has a critical flaw.

For this and other reasons I prefer pair programming as an alternative to code reviews. Work progresses faster when two people do it (vs slower when one has to finish it and then another review it). You actually have two people understanding what happened. And you can avoid all of the drama because problems get fixed as the solution is being designed and written.

And yes, this is a good (or at least better) time to teach. Though I try to not be preachy and instead prefer to demonstrate how I solve problems and comment on why.

Same above goes for anyone who believes they have "finished the work" before getting any reviews.

There is your idealised view of how software development should work and then there is the real world. I prefer to stay grounded in the real world and figure out actual ways to help people succeed rather than telling them how they should be performing and firing if they can't meet my standard.

Whatever you think about what people "should be feeling", the truth is when they made their last commit almost everybody feels they finished their work and when it is thrown back to them they do not welcome it. I have never seen a hiring process able to only pass people who are happy to get review comments requiring them to review large part of their work.

Works goes faster when you work with another person even if for the fact it is harder to procrastinate.

And problems are always easier and cheaper to solve the earlier in the process they are caught.

Then there is the simple fact that a well executed coding review will take significant portion of the time it took to make the change. Most coding reviews are really only cheaper because the reviewer is just skimming the code to see for obvious faults. So it is not really apples to apples comparison.

Sorry, but that's not a brag. Quite the contrary, actually.

On the topic of code reviews, I think the style varies so greatly between teams that it's hard to generalise. I've had teams where the others were very eager to learn and ones where they'd rather not.

> I find the fact one person actively makes the other person “think”, extremely condescending

The only thing I learned from this and the other example given in the blogpost is that some stranger named Greiler thinks that "teacher" means "person who tries to be kind."

The message might work better if it were reframed as "code review feedback should be direct and succinct."

What if you find yourself giving so much direct feedback that you're basically rewriting the code via comments, time and time again?

Feedback or instruction that's not super direct has its place - we have to foster independence somehow. If I'm in a lead position, I have to be able to ask you to go work on a bug or think about something on your own, even if I could probably figure out an answer in a short period of time myself.

Trust must always be in the room in order to ask someone to work, whether it's in code review or elsewhere. If that trust is not there, more direct feedback/instruction can help rebuild trust, but it is not the end-all-be-all.

To tie this directly to the example: there may be points where I don't give a suggested name or solution in my comment simply because I haven't thought of one, and the reviewee may need to be able to accept that without them thinking I'm being passive aggressive. (I would do my best to communicate that context in those instances.)

Otherwise it’s kind of like - “why didn’t you just do this yourself if you had something so specific in mind?”

What, exactly, is wrong with "I think the current name of this function fails to encapsulate what it does. I think openRequest() would be a better name."

[1] At least, that's how it comes across when reading a lot of his articles.

The suggestion is to not make vague statements and instead say what you mean, not that code reviews shouldn't be educational.

Like when I was a dev and a senior was reviewing my code - I really wanted to know how and why they thought so I could learn, and I made that known.

Emotions and defensiveness are a thing and you can't change how open someone else is, but to the extent that you can keep yourself as open to and welcoming/soliciting of feedback, the faster you grow.

> “I had a hard time grasping what the method does. What about changing the method name to openRequest() to make the methods objective clearer and improve code readability?”

That suggestion requires grasping what the method does.

In the original article the feedback is given after the submitter had explained.

I've found them to be invaluable opportunities for knowledge exchange

But...this article isn't about not teaching. It's about not trying to be coy, hinting that the code needs some unspecified change.

Absolutely do tell me how my code is flawed, and how it could be better. This is what I would call teaching.

He considered himself an excellent code reviewer. I've never worked with the author of the article, but the line "I'm not sure I understand the whole idea, but could you explain what this method does?" Remember my old boss. It started like that... and the next few days were a review hell.

Respect people and their time. Also be open to sugestions and for reviewers not every suggestion is a must, make clear which ones are.

> It’s not bad to “teach” in code reviews, but it should happen on an eye-to-eye level.

> But first, let me show you how I’d phrase the feedback for this example:

> “I had a hard time grasping what the method does. What about changing the method name to openRequest() to make the methods objective clearer and improve code readability?”

So the headline is just wrong, it's about not being a dick or vague when writing code reviews.

It all depends on who the parties are, their relationship to one another, their relative knowledge of the tech stack, application and problem domain, and the nature of the code being reviewed itself.

Your reviewing style should ideally be tailored to the above contexts. Conversely, if you use the same exact process everywhere, you’re probably doing it wrong.

The worst part about this is that it forces more unnecessary communication in the code-review process...

If you're reviewing, then say what you think needs to change and why...

Wtf. I’m sure glad I cut my teeth before advice like this became widespread

PS. But, titles aside, do we actually want to do teaching during code reviews? There are many activities when teaching and doing are better kept separate (like, you don't want to teach your partner how to dance while you're dancing). Do code reviews fall into this category? Should we consider them a doing phase or a teaching phase?..

Well, that's how you teach someone how to dance...

A good friend of my wife once did this to me at a wedding. She had started a pair dancing hobby maybe six months earlier and was very enthusiastic about it. When I asked her to dance, I didn’t mean I wanted a lesson. It was so out of sync with my expectations that I left with barely an excuse in the middle of the song.

So there are exceptions to the rule

Yes.

Expert intuition is a tacit skill one can only learn on the job. By getting feedback from an expert in real-time. It cannot be done as a separate exercise. There have to be real stakes and the work has to be real.

You wouldn’t expect a surgeon to not get feedback during their first surgery would you? But you also wouldn’t want them to cut somebody open just for learning in the absence of a medical need.

We even call this “supervised learning” when a computer is doing it.

This is quite common in teaching, and assessment, to the point that some people think it's synonymous with teaching. It really isn't a good way of teaching, and I gather that modern teacher training teaches you not to do it. https://betsysneller.github.io/pdfs/Labov1966-Rabbit.pdf

> He tells me that the teachers had already decided that many of the school children didn't have any language at all: they didn't know English and they didn't know Chamorro. When he asked them how they knew that, they described the very same kind of testing procedure that I have observed and reported in mainland schools. […]

> The children's response to this test, in general, was to say as little as possible. […] James is one of the most talkative children in the group. Others said much less. Some were paralyzed into silence by the request for display: […] To all these questions, Eunice presented a stubborn resistance. Finally, she produced a minimal response to the teacher's verbal bludgeoning: […] The teacher-tester is a pleasant person when you meet her face-to-face as an adult. […]

> A third characteristic of adults' talk to children is deliberate and obvious lying. The teacher-testers frequently try to force answers to known-answer questions by claiming that they don't know things which they plainly do. As the children follow the strategy of saying as little as possible to stay out of trouble, they frequently answer with "Uh-huh" or a shake of the head. The teacher could simply point out that the tape recorder wouldn't pick that up. But instead she says, "I don't know what uh-huh means."

---

In fact, the author's preferred code review:

> “I had a hard time grasping what the method does. What about changing the method name to openRequest() to make the methods objective clearer and improve code readability?”

is a much better lesson than the "teaching attempt" it replaces. I would call teaching the primary purpose of code review, with the resulting codebase improvements a useful (but necessary) side-effect. The alternative, of just silently fixing the code, is worse because it doesn't stop the same mistakes being made again.

The Socratic method for teaching is actually quite good, and it can be employed without condescension.

Socratic Method is hard to use without coming off as patronizing, even if you think you're being careful.

If you're genuinely jumping in to ask real questions to understand the problem, that's great.

Most of the time when I see people use Socratic Method in the workplace it's because they think they're doing the other person a favor by asking leading questions instead of communicating directly. For the person on the receiving end, it becomes a game of navigating the questions and delivering the answers you know the person wants to hear, all done as delicately as possible to avoid disturbing their sense of superiority.

Once you start doing this, every question starts to feel like a loaded question. Is this person genuinely asking my thoughts, or is this question another test to see if I agree with their secret answer? Am I okay to express a differing opinion here, or will I trigger another round of patronizing questions if I give the wrong answer? Is this person asking questions because they don't know, or because they think they know better than me and want me to realize I'm wrong?

It's almost always better to approach the conversation as a discussion between peers. If you go in with implied seniority structures or student/teacher methods (including Socratic method) then it stops being a conversation among peers and starts to feel like another social hierarchy game that must be carefully navigated to avoid upsetting your elders.

Just talk to your coworkers like collaborative peers, even if they're more junior than you. If you want to communicate something, say it directly. Don't ask questions and try to get the other person to guess the secret answer you want.

My boss and I had an open discussion and it may be they just need more practice - coaching is a skill like any other and just because you took a two day class doesn't mean you're an expert yet. So we accepted that while he's more experienced than I am and my mentor in many things, he's junior in "coaching" so we sure working on it together :-)

In the workplace, Socratic method just feels like an unnecessary power move. Someone is trying to cement their role as teacher and the other person as student.

I had a manager who liked to use the Socratic method for everything. He communicated everything in the form of questions. If you gave the "wrong" answer, he'd give a sharp sigh and then rephrase the question, giving you another change to give a "better" answer.

This method was mildly annoying when he was right, but it was completely disastrous when he was wrong. He'd often arrive late to a situation that people had been dealing with for months and assume he knew exactly what was going on, better so than the people involved. He'd start his Socratic questioning, but you weren't allowed to explain the context or how you arrived at a solution. Your only option was to navigate his Socratic questioning until you could gingerly explain that he was missing a key constraint, or that we had already tried that, or that he had received bad information, and so on. It became a tool for him to control the conversation and put you in your place at the same time.

It was a very sad time in my career. I'd arrive to every meeting feeling like I was about to play a psychological game of "guess the right answer" as I navigated the Socratic questions until we could get him to reveal what he was really thinking, or why he was so frustrated, or why he thought we were wrong, or any other number of issues that he just wouldn't tell us.

Now I say explicitly what I think is wrong and what needs changing.

This too can come off badly so I limit reviews to one or two remarks. I also build a relationship with these engineers and explicitly explain how I do reviews different nothing that while I give you solid direction, whether you take my advice is entirely up to you to evaluate and that it is 100% ok with me if you explain why I'm wrong and you're not changing it. - the purpose of my reviews is to catch problems you haven't thought of, if you've already thought about it and made a trade off that's your call to make. (And I trust you to make it)

Translation: I'm too old for this tit for tat shit and have bigger hills to die on than where you put your whitespace.

I don't know how that relates to the Socratic method. But I think it's useful, if done honestly. It's like a little ritual that you do together and it serves both sides as a guideline in the moment, but also helps to converge to a common way of communicating things and writing code.

I seriously doubt a PR is the right medium for the Socratic method, or that trying to employ it in that venue can sound anything other than condescending.

PRs are the end result of the developer's work to address an issue. Communication through a PR is asynchronous and inefficient wrt time. Engaging in gratuitous chatter in PRs actively blocks the developer from delivering his work. Pushing for needless iterations with cryptic open-ended questions where you force a developer to be on the defensive reads as if you're standing in the way while gratuitously questioning someone's ability in a very public way through a medium designed to persist discussions til the end of time.

If a PR has an issue just do the right thing and go straight to the point. If you feel the need to get into what you feel are teachable moments, reach out to the developer through your choice of direct chat.

Maybe I'm just too damn old, but I don't like this trend.

When code reviews were in person, as a senior person, I could deliver feedback without that feedback becoming a public albatross stuck around someone's neck. We all screw up and miss things. Sometimes we don't write the best code. Sometimes we don't know something that should be universal knowledge.

My correcting your mistake or lack of knowledge shouldn't get recorded for all to see.

With the way things currently are, I now have to do two code reviews. One to correct actual problems and teach/mentor and the other for the public checkin to the system codebase.

Emails that have a long list of CCs suddenly become a political hothouse of "If I say X in front of Y it will seem like a criticism and they will clam up and then the whole thing becomes something the "positional authority" has to adjudicate not an experience led discussion"

I feel that this problem is the very reason FOSS mailing lists (ie Linux) were so brutal - it's that or you risk lack of clarity.

I know I sound like someone complaining about woke snowflakes, but there is a line somewhere around here and I wish I knew where it was

Heh, I actually ran into a funny situation because of permanence of code review.

One time I was onboarding a new employee (call him Bob) to how pull requests worked in GitHub, and I wrote up a sample one from his computer. Just to be silly, and because I didn't realize the implications, I said "okay and you fill out the body, describing what you did. As an example, let me just put in some filler text, 'hey, you people suck'."[1]

I was planning to delete the obvious-garbage PR, but I didn't realize ... GitHub doesn't let you delete PRs, only close them! And it triggers emails!

Mercifully, no one said anything. But then months later, another co-worker (call him Charlie) was venting to me about what he didn't like about Bob: "And, another thing, one time, that asshole wrote up this pull request, where he said, hey, you people suck!"

So I owned up: "Oh, uh, Charlie, that ... was actually me. I was writing a dummy pull request to onboard Bob but sent it by accident."

Then Charlie said, "Well ... he's still an asshole!"

[1] I think I wasn't planning to submit it at all, but after a while Bob probably wanted to see it in action and I forgot to remove that part. (And yes, I also now make sure to use more innocuous filler text.)

Is this a thing you’ve seen or just something you’re afraid might happen? If the former, I would say that’s an unhealthy work environment. If the latter, then why point it out?

> "Let's say someone makes the argument that the Socratic method has major flaws related to how it is applied in practice, and that it's better to explicitly state issues with someone's approach or line of reasoning? How might you respond to such a disagreement among your cohort of senior engineers when it comes to code review practices?"

It's always going to come across as "Look I'm patiently trying to lead you arond to the conclusion that you made a mistake or don't understand things correctly, because trust me, I know better."

Also, a lot of junior people know all about how this game is played and will play along, faking the whole 'dawning realization of the error they made' thing and expressing appreciation for the master's wisdom, while secretly thinking, 'am I going to have to go through this every time I make some mistake or other?' I certainly spent quite a bit of time doing that in the past.

I think it's just a lot faster and more efficient to point out errors and issues as you see them, and any decent engineer will absorb that information pretty quickly.

1. You post a question with non-specific action intended to stimulate them to critically think and fix a defect which doesn't exist. 2. The bewildered person tries explaining what is going on blindly, not sure what you are hinting at. 3. If the general explanations connect, maybe you see your mistake and resolve otherwise you explain what you were thinking the defect was. 4. The author explains the lack of defect. 5. You apologize and resolve.

If the person has any confidence at all then they will consider this a demoralizing possibility.

Even if the person didn't make a mistake if you know there is an issue and are withholding that from them because you think them figuring it out by being stimulated from answering your question is benefitial to them, then you are not being immediately helpful in an asynchronous process that has a tendency to drag on. Our society would not be capable of the things it was today if everybody needed to reinvent the wheel frequently as a learning experience; direct communication is a strength we have.

All I am saying is, go sit down with them if you want to use the socratic method. It is multiple times more effective then.