> I’m not sure if I understand the whole idea but could you explain what this method does?
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.
This is not a question or discussion that should go in the code review record, IMO. If you do have this question during a code review then directly go have a chat with the person who wrote the code then go back to the review. It also quicker and easier.
Great article, I like the thought here. I've fallen for this myself, trying to "teach by asking questions", but in reality just come off as patronizing or disrespectful to your counterparty. That kind of thing is a lot more suited for a discussion where it's legitimately important that someone come to the conclusion for themselves (eg, a political argument). A code review is different, both parties already have an implicit obligation to be receptive and open to feedback. You can be direct, and talk somewhere else about the "grand lesson" if you want.
I disagree with the title but found myself agreeing with many points in the article.
“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!
The word "discussing" is doing a lot of work there and does nothing to distinguish it from condescension. Knowing something is one thing, being nice about it is quite another. Opposing them with a "but," like "hey that just goes with the territory," is not actually addressing the issue.
Yeah, I think the title is a little provocative to get you to read, but I ended up agreeing with it. Maybe "don't try to be a teacher during code reviews" is slightly more precise?
Yeah, and it doesn't take much to convey that this is a conversation between peers. A couple simple changes I get a lot of mileage out of. Where once I'd have written:
"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"
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.
Agreed, this isn't about teaching. It's about people who stroke their own egos when they should really be instructing. The example comment doesn't tell you anything useful except that the commenter might not understand what the author is trying to do. That has no call to action, no indication of a problem, and basically nothing useful to the author. It is a complete waste of time ...
Clear and concise is often received as short and terse. I personally think people as a whole need to learn to receive communications better, versus expecting the author of said communication to know how to tailor their message for your individual preferences. So the whole premise of this article is focusing on the wrong side of things in my mind.
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.
There’s no link to the original medium article but I have a feeling the author was female or some other minority in the industry. I might be totally off base with that though. I agree that the wording is very cringy, but if I may offer one defence: I also learned to do this sometimes because otherwise juniors wouldn’t listen to me at all. You had to make them think it was their own idea or they were magnanimously granting you, an idiot (notice how they apologise for being slow), a rename to help your tiny brain understand. Otherwise you’d be stuck in an insane week-long code review with the junior furiously dismissing every single instruction until you looped in a coworker with more social power. I see this article is also by a woman and I’m happy that she’s in an environment where she doesn’t have to do that. I don’t think the other article’s wording is praise-worthy but I do think it’s a somewhat rational adaptation to an environment of hostile coworkers where you need to make yourself submissive to get anything done.
I mean these days people take offense when you link them to the documentation or propose a better way to do something. They have too much pride to accept that they might not be hot shit.
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.
Isn’t that more “don’t give vague feedback” or even “don’t misdirect the reviewee”?
“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.
I think it's more of the author having a misguided opinion of what teaching is. I've had a lot of good teachers in my upbringing but not one of them has ever thought that a vague/condescending question like "I’m not sure if I understand the whole idea but could you explain what this method does?" would be an effective teaching method.
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?
Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops.
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?
> Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops.
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...
The ones who are currently teaching the kids right now?
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 was actually surprised when I looked up the pay bands for my old high-school and found that (head) teachers were earning near programmer salaries (this in Europe).
Good teachers teach you in the first place. The test part and telling you what you had done wrong comes later.
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.
I think the title wasnt totally clear but it's a totally valid use of the word. Its making the distinction between telling someone the answer and leading them to an answer.
It’s usually very transparent when someone is pulling this tactic so I like to cut to the chase by providing some token explanation. At the end I then ask “did you have any changes you would like to propose?” Tends not to happen again after that.
I agree with the everything the author says in this article. But their advice includes teaching during code reviews, so the title is misleading. Explaining the 'why' of a code review comment is teaching, nothing wrong with that IMO.
Not sure if I agree with this. Maybe because at my place the culture is much more direct. It's more about what u r 'teaching'. If u r right and proposing a better solution and u back up with evidences ppl r more than happy to consider ur comments. If u r proposing something like an alternative, then that's not a teaching and would turn into the discussion.
There is no playing such games in our code review. Saves everyone's time and directs to the points
100% agree with this, especially the part about slowing down code reviews. Don’t be condescending but give your opinion directly, asking if it makes sense. I personally like “what do you think about renaming this… was thinking because…”
> Why care much about "style" if you already know intention clearly ?
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.
How you develop people using code reviews depends on the person you're developing. How you ensure quality code depends on who's submitting the code. A lot of people aren't receptive to feedback or change requests until you make them think through the issue a bit more. It's imprudent to make hard and fast rules about code review etiquette, but a lot of the suggestions here count as useful guidance to consider.
I’m not sure if I understand what this article has to do with teaching?.. Oh, I get it now, sorry for being slow! Do you think we could rename it to "Don't be an asshole and lie about (not) understanding things" or something along those lines? :)
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?..
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.
I just remembered I did recently ask someone to teach me a dance ("just off" the social floor), but 1) it was in a crowded bar just off the dance floor so it's less in the way than the onlookers 2) it is a notoriously easy dance 3) she knew I was good at dance 4) I picked it up in 45 seconds 5) we immediately went on to the dance floor and finished the song together with no further instruction.
> Should we consider them a doing phase or a teaching phase?
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.
> Don't be an asshole and lie about (not) understanding things
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 whole main short-term, medium-term and long-term points of code reviews is to teach. Teaching is a gift. It doesn't mean you have to slow things down but it's always a gift. Anything implying otherwise negates the experience of previous engineers helping even if it seems "difficult" and takes extra time.
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.
Seriously, I read the first sentences and got so distracted from the pop up. Wtf is that animation? The close button way too high (on iOS), so I had to scroll up again and then I had to search where I left off. I just closed the tab instead.
Yeah the high-contrast aspect (very dark background overlay and bright white animating block) makes it especially startling! That was pretty nuts. I also immediately closed the page, even though I wasn't done scanning/critiquing the content.
I tend to agree with all of the principles here, with some caveats.
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.)
I’m with you. If I don’t think a chunk of code is readable, I’m not going to rewrite it all for them off the bat. I’ll just say that it’s not clear and could probably use some cleaning up. And they can either push back, or do something on their own, or ask if I have anything in mind, or seethe silently and ignore me. If it’s complicated or seems like they’re struggling then I’ll ask if they want to pair on a solution.
Otherwise it’s kind of like - “why didn’t you just do this yourself if you had something so specific in mind?”
This works better if you both are in roughly the same time zone. It's not so great when you have to play "guess what the reviewer wants" with one day of latency between iterations of the review.
When one reviews another person's PR, it's as if you are reviewing your own. This is not a mere superficial verbiage; a few times, I'll review someone else's code, and actually block on small modifications and comment on them asking for more details, only to discover that the small change was to something I myself wrote and I forgot it so deeply as for it to appear entirely new. As one is kind to oneself often, one needs to be kind to the others, and trust them. It starts from there, and you co-learn and re-learn (which I think is a subtle point in the article), since whether you wrote it or them, it does not matter.
I really resonated with the way you frame it. For me it's absolutely key that ego doesn't get in the way of teaching/learning and the quest for improvement. People need to feel safe so they don't feel bad about having their output critiqued in front of co-workers. They should relish in it! Teaching is a gift! We're all perpetually learning, forgetting and making mistakes. The enemy is the idea that we need to maintain an image of getting things right all the time.
I use junior engineers’ code review submissions as an opportunity to teach, and any senior engineer who doesn’t is committing professional malpractice. They have to learn, and that requires someone (in the case of a code review) to teach them. One can do it without being condescending, though
The Socratic method for teaching is actually quite good, and it can be employed without condescension.
I think the real problem here is that because everything is going online-only, we're falling into the trap of making everything public and recorded forever.
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
I’ve written plenty of bad code before, and some of it was released and some of that led to tricky bugs. I don’t feel particularly ashamed of having written bad code though. Maybe I’m thinking of a different kind of feedback? Like, hopefully the worst problems can be figured out before much code is written, and if someone made some asshole critical comment, I would probably consider it reflecting poorly on them rather than me (indeed I find the permanent record of the comments I left and since regretted weighs more heavily than the record of the comments received – or the poor code committed). But I guess different people react to feedback differently, especially critical feedback and maybe you’re imagining some kind of ‘the whole thing is totally wrong and terrible’ discussion? But that feels to me like a case where a bunch of blame belongs to whatever allowed a bunch of totally wrong code to begin to be written.
I agree that submissions that bad shouldn't have a permanent record. If I see something egregious I'll ask the person in person, or via our chat software, and make a vague comment like, "Please make the changes we discussed over lunch," or whatever, in the comments of the submission.
In my experience, the problem you’re pointing out is a non-problem. I’ve never done an in person code review in my life that didn’t take place in an interview context. Yet, somehow none of my mistakes in writing code (and there have been many) have never become “a public albatross stuck around [my] neck.”
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?
>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.
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.)
Eh, even the socratic method is quite patronizing in many situations.
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
100% Agreed. Just say what you think and why. The Socratic stuff works really well if you're making up a pretend conversation where you get to write both parts, and every question gets the perfect answer, and it all gets to ends with perfect enlightenment. In the real world, your questions probably suck, and the answers probably suck more. Nothing is more obnoxious than enduring someone badly doing "the socratic method" on you.
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".
I ask a lot of questions on code reviews as it's usually less easily perceived as conformational and I have come to assume that I'm missing context when something is really off. Assume that coworkers are competent but may either have or lack context. If your coworkers are genuinely incompetent, then a new position might be better than fighting though PRs.
I don't know how easy it is to avoid coming off as condescending. For example:
> "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.
> The Socratic method for teaching is actually quite good, and it can be employed without condescension.
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.
Yes, and not every kid wants to be told what to do by their parents. But it still happens, because that is part of the parents' role. As it is with senior developers.
You write that as if every -ism contains contains an automatic and universal value judgement. For those of us who don't subscribe to that model, do you think you can rephrase your argument without isms?
Do you know why children don't like to be told what to do by their parents? It's not because they can't do whatever they want. It's because as a child, your agency is taken away. You have no choices but those given to you, no information but what little is given to you. You live in a world of someone else's rules and decisions, someone else's tastes, someone else's lifestyle. You are forced to live the life someone else wants, rather than your own life. And you are often given restrictions not because it's what's best for you, but it's just what's convenient for the person in power.
Children are often not given the benefit of the doubt. Things aren't explained to them, and they're lied to. Much of the time parents don't even do what's best, but instead whatever they prefer. The child may actually be fine with what a parent wants. But rather than give the child the knowledge and tools to make their own decisions and come to their own conclusions, the parent forces their child to become subservient, removing their ability to be a free human being, making their own mistakes, achieving their own goals. Children often grow up stunted by years of biased information, and the inner working of the minds of others who have grown up in the same system.
Paternalism is wrong because it's wrong to think you know what's good for others. You may know what's good for you. And you may have some experience that tells you some potential consequences of specific actions. But by pushing your own desires and limited knowledge on a child, and not permitting experimentation, growth, or making one's own mistakes, the child does not really grow. They just become a poor copy of the parent, and just as limited. Later they internalize all this and try to justify it by doing the same thing to others or their own children. But it's just a coping mechanism for the trauma they experienced as a small human with no rights.
If you actually respect your co-workers, do not treat them like children. Do not condescend. Do not limit them. And do not prevent them from making simple mistakes. Give them information, teach them, sure. But only if they want to be taught. Not because you think you have some special right to tell people what to do, or force people into situations they didn't consent to.
As a senior myself, I am happy to share information and help people learn and grow. But I would never tell a junior what to do, or lecture them, or prevent them from making mistakes. Everyone deserves to come into their experience in their own way, and be an equal member of the team.
> 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.
Yeah ; I'm super open to coaching as opposed to mentoring, but last few years, when my managers tried to employ the coaching / questions methodology, it was obvious and painful : they did not feel like open questions to discover something together ; they felt like fake questions that had a "right" or rather "expected" answer, so it did not feel at all like "me figuring things out for myself " but rather"me trying to guess what I'm expected to say". Typically after a few frustrating minutes I just ask for straight feedback - I am happy and excited to receive constructive feedback and lecturing.
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 :-)
I had some college professors who used Socratic style in class and I enjoyed it. However, that was literally a teacher/student relationship that I signed up for. Furthermore, the professors were usually genuinely curious about my responses and wanted to explore them when they didn't necessarily agree with the expected answers.
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.
+1 I gave up on the "teaching" years ago, it never did anyone any good and wasted a lot of time.
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 should also clarify that I am always clear on exactly why I am suggesting something, and the level of importance I place on the change (though usually, if I'm commenting, it's relatively important, if not, I just don't bother mentioning it)
I think there's value in having a small set of 'goto questions' or maybe rules that are well known and regularly used. Then you can go over them together in the case of a review, a debugging session or to clear up a problem.
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.
The Socratic method is bad over asynchronous communications because if you, Socrates, made a mistake then it takes 3-5 iterations to clear that up.
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.
If you're looking at someone's job title before making a review I'd say you're doing it wrong. Just point out things that are wrong (actual bugs) and things you think could've be done better. If there are no bugs it's probably ok to even approve it already, unless things were really poorly written. It shouldn't matter whether the person submitting the PR is a junior or the highest ranking developer in the company. Code is code.
"Socratic" method (which was used in a book, not in actual teaching, IIRC), works when the student wants to figure something out on their own but with guidance, like "You Could Have Invented X" blog posts, or tutoring math. It doesn't work when the "teacher" forces it upon an unwitting "student" to trick the student into discovering they are wrong.
> any senior engineer who doesn’t is committing professional malpractice
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?
s/teach/be petty/. Nobody cares about a method name and if I care, I can send my own cleanup change later instead of holding up work of others. A good compromise is to approve and still suggest a different name in comment and then trust competence of my coworkers to decide for themselves.
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.
Yeah, totally agreed. My general principle on method names is: suggest an alternative, but it's a purely-optional suggestion. Sometimes I can think of a far more effective name for something, and I'll suggest it, but say "not required, just suggesting". There are a ton of things in code reviews that could be improvements, but are also not a big deal, or quite subjective. Then there are the things that seriously affect code quality and future maintainability, which would be the kind of thing that should hold up the review until improved or corrected. In either case, being completely clear and straightforward is always a solid approach. Asking weird rhetorical questions (as opposed to clear and direct questions) does not help the process and generally elicits uncertainty and self-doubt in the code submitter.
Considering context is helpful also. Like perhaps they implemented some function in an odd way because other similar functions in the existing library are also that way.
I think the example is too contrived. Like you said, make a non blocking comment about the name and be done.
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.
A misleading method name can cause serious screw ups down the line. At best, they make reading through the code later much more confusing. It's not petty to ask for accurate and comprehensible method names.
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.)
Often when reviewing a change I will frame my feedback in the form of a question, not because I am trying to be a Socratic jackass but because I'm not sure. Only a fool thinks they are an expert on C++ so when I say something like "does this const-qualified variable declaration in namespace scope necessarily imply internal linkage?" it's because I want to know, not because I already know. And if it's not clear to me, it also won't be clear to the next person who reads it.
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.
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 author's example doesn't work. It's too retrospective.
> “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.
Clickbait title but I generally agree being direct in code reviews is great especially if it reduces the feedback loop & doesn’t drag out the code review process.
Not to mention when my mind comes up with the logic to solve a coding problem, it’s absolutely not in a format that lends itself to explaining to another human
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
It has something to do with "making the person realize the problem by themselves", because some people get offended if you directly tell them "do Y because it's better", but I completely agree the way is being done there is not effective.
I hate, hate code reviews. Seems to attract crazy people.
An example:
My old boss did a code review on my code and sent ALL CHANGES via chat. Not instructions, code. "Please do it". I did. And in the reassessment he hated the code I wrote, which was his. He publicly cursed me out on the review tool. It is written for anyone to read years later.
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.
303 comments
[ 3.0 ms ] story [ 265 ms ] threadMaybe...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.
“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.
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.
“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?
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.
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.
There is no playing such games in our code review. Saves everyone's time and directs to the points
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.
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.
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.
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?”
The Socratic method for teaching is actually quite good, and it can be employed without condescension.
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
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?
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.)
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?"
> "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.
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.
Children are often not given the benefit of the doubt. Things aren't explained to them, and they're lied to. Much of the time parents don't even do what's best, but instead whatever they prefer. The child may actually be fine with what a parent wants. But rather than give the child the knowledge and tools to make their own decisions and come to their own conclusions, the parent forces their child to become subservient, removing their ability to be a free human being, making their own mistakes, achieving their own goals. Children often grow up stunted by years of biased information, and the inner working of the minds of others who have grown up in the same system.
Paternalism is wrong because it's wrong to think you know what's good for others. You may know what's good for you. And you may have some experience that tells you some potential consequences of specific actions. But by pushing your own desires and limited knowledge on a child, and not permitting experimentation, growth, or making one's own mistakes, the child does not really grow. They just become a poor copy of the parent, and just as limited. Later they internalize all this and try to justify it by doing the same thing to others or their own children. But it's just a coping mechanism for the trauma they experienced as a small human with no rights.
If you actually respect your co-workers, do not treat them like children. Do not condescend. Do not limit them. And do not prevent them from making simple mistakes. Give them information, teach them, sure. But only if they want to be taught. Not because you think you have some special right to tell people what to do, or force people into situations they didn't consent to.
As a senior myself, I am happy to share information and help people learn and grow. But I would never tell a junior what to do, or lecture them, or prevent them from making mistakes. Everyone deserves to come into their experience in their own way, and be an equal member of the team.
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.
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?
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.
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.
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.)
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 suggestion is to not make vague statements and instead say what you mean, not that code reviews shouldn't be educational.
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 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.
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
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.