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.
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.
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.
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?
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.
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.
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"
There are better places, and ways, to teach junior developers than long, painful code review processes.
I'm not talking about that.
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.
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.
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.
I do like some of this author's articles on code review but I think they emphasize too little that the author of the change is not one of the interested parties in the review. The review is the opportunity for the organization to defend the interests of future maintainers of the code. The interests of the person proposing the change are a distant second and they should be ready and able to either advocate for their decisions or acquiesce to requests for changes.
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...
So now you’re making me sit and “look stupid” because I have to actually parse out in human language why I think the way I think, meaning I have to sit and reason out why I did something
This is such a terrible approach
Wtf. I’m sure glad I cut my teeth before advice like this became widespread
There is no playing such games in our code review. Saves everyone's time and directs to the points
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...
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 :-)
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.
Interesting. Why not turn it around and say that any junior engineer who doesn't use comments left by his more experienced colleagues as a learning opportunity is committing professional malpractice?
For example: underlying does a task that is correct and valid but not what the higher up envisioned or hoped for. In this case you’re just going to come off as a prick for “gently guiding me” to a conclusion I disagree with, but which I’m not allowed to (non confrontationally) articulate as you’ve corralled this conversation down one narrow path based on your personal preference
As someone who while I was a junior was starved for someone to actually teach and mentor me, I’d say this scenario played out much more often than one where I was given valuable education. yrmv
The main problem with junior engineers is that they have narrow views of the world. Software is complex. Being experienced, is mostly about earning hard won scars through mistakes, missteps, and rewrites. It's tough to convey the scope of impact by just asking them questions. It's much more guiding to just give them what they don't yet have!
I'd much prefer: "Oh, man, so this looks like it shouldn't matter, but let me tell you how I once took down prod for 3 hours by doing this same thing. I can show you how it can fail in this really subtle way".
to: "what is the nature of being?"
Quotes from article
> It’s not bad to “teach” in code reviews
after example of "proper" review
> The learning in this type of comment
Should absolutely "teach" during code reviews. Developers should always be teaching and learning from each other.
Maybe...It's going to depend on the culture. If you have a passive aggressive culture, something like this is a good fit. Otherwise, to me, you're adding friction.
Yes, it might be better to make the submitter think. But if you have to be anti-truth-seek to do it, that's a net loss.
That would perpetuate the passive aggressive culture.
I can handle nit-picks, compliments, curiosities, etc, but post them in Slack, not in the middle of my work. I wouldn't nit-pick you during a presentation ("this use of bullet points isn't very efficient, I would do it a different way") or critique your wardrobe at the water cooler. Don't do it to my code when I'm trying to get work done.
You'd do Y, and then he'd complain about the changes that occurred because of Y, so now do Z. And so on.
I'd throw my PRs up as drafts early on, and invite his feedback as soon as possible to try to avoid this, but nope, it all came when the PR was ready for merging.
In the end I decided to work in an area that this lead didn't like and didn't know about, just to get code merged without spending two weeks in review.
I'd say that you actually did, that's why it wasn't a yolo-merge
the first is annoyed and frustrated. It says "Dave's proposal is just an irrelevant nit pick. Who cares if I make that change or not? it won't affect the product in any meaningful way."
the second is relaxed and easygoing. It says "Dave's proposal is just an irrelevant nit pick. Who cares if I make that change or not? it won't affect the product in any meaningful way."
I listen to the second, accept the revision, and move on.
In worst case, the submitter will get "ignored" later with good advice and it's bad for him and the team itself.
You don't need to pass code review to get merged. There's always a DoD for it.
What to do in this case ? Just answer it the way you feel good for the team. Don't care much about style.
In my case, i'm always grateful for being taught by teammates. I don't care much about "teaching" or anything like that. Intention is all you care.
Because it may have taken me 5 minutes to understand what the function did because its name misled me, and that was with the benefit of the context of the PR. Trying to make sense of it while debugging something broken 6 months later is going to take even longer, and getting the naming right may pay significant dividends later on.
