23 ms·

Don’t teach during code reviews

196 points by trashymctrash 4y ago

chiefalchemist 4y ago
> 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.
- mytailorisrich 4y ago
  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.
- gtirloni 4y ago
  > If you have a passive aggressive culture, something like this is a good fit. That would perpetuate the passive aggressive culture.
  - chiefalchemist 4y ago
    That's exactly my point.
aquinas_ 4y ago
[flagged]
extr 4y ago
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.
sesuximo 4y ago
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!
- tccole 4y ago
  I think that goes to giving a why with a suggested change and giving a rough sketch.
- rhizome 4y ago
  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.
- ketzo 4y ago
  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?
  - nonethewiser 4y ago
    What's the difference? Presumably teachers teach.
- rictic 4y ago
  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" Clear, actionable, refutable
HL33tibCe7 4y ago
The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.
- coffeebeqn 4y ago
  I just want people to be clear and concise. I hate the vague shit sandwich rhetorical questions. 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
  - nonethewiser 4y ago
    I agree but I think honestly asking "why did you do it this way?" is important sometimes. The answer may completely change your suggestion. For the contrived example about renaming a method, no I dont think you should ask why they named it that way.
    - coffeebeqn 4y ago
      That is a straight question and definitely valid.
  - conductr 4y ago
    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.
Someone 4y ago
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.
- password11 4y ago
  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?
  - thinking4real 4y ago
    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?
    - wk_end 4y ago
      > 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...
      - nonethewiser 4y ago
ratherlongname 4y ago
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.
gloryjulio 4y ago
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
mephitix 4y ago
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…”
azov 4y ago
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?..
- coldtea 4y ago
  >like, you don't want to teach your partner how to dance while you're dancing Well, that's how you teach someone how to dance...
  - throwawaymaths 4y ago
    Gp probably meant on the social dance floor which is generally considered to be taboo, and doing so is a mark of either a jerk or a noob.
    - pavlov 4y ago
      It really is surprisingly infuriating. 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.
      - deleted 4y ago
        [deleted]
      - throwawaymaths 4y ago
        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. So there are exceptions to the rule
revskill 4y ago
Why care much about "style" if you already know intention clearly ? 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.
- yuliyp 4y ago
  > 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.
languagehacker 4y ago
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.
tmsh 4y ago
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.
phreack 4y ago
Off-topic: 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.
- hannes0 4y ago
  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.
  - napsterbr 4y ago
    Yeah, same here (iOS). I didn't bother trying to figure out where the "close button" was though.
  - thinking4real 4y ago
    How else are they going to get spam email lists based off poorly advised articles?
    - rhizome 4y ago
      Flame-bait gets good traffic, can't let that engagement go to waste!
- amatecha 4y ago
  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.
koof 4y ago
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.)
- kdazzle 4y ago
  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?”
  - lmz 4y ago
    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.
    - doutunlined 4y ago
      Your code reviews are doomed regardless if the feedback cycle is 1 day.
why-el 4y ago
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.
- johtso 4y ago
  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.
sidlls 4y ago
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.
- deleted 4y ago
  [deleted]
- bsder 4y ago
  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.
  - lifeisstillgood 4y ago
    This. 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
  - dan-robertson 4y ago
    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.
cat_plus_plus 4y ago
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.
- amatecha 4y ago
  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.
- tyingq 4y ago
  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.
  - nonethewiser 4y ago
    That's why _honestly_ asking why they did something a certain way first can be a good idea.
jeffbee 4y ago
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.
foota 4y ago
I would suggest reading the article before commenting. The suggestion is to not make vague statements and instead say what you mean, not that code reviews shouldn't be educational.
Scubabear68 4y ago
This post is too one-size-fits-all. 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.
politelemon 4y ago
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.
CrimpCity 4y ago
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.
gitgud 4y ago
> I’m not sure if I understand the whole idea but could you explain what this method does? 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...
- thinking4real 4y ago
  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 This is such a terrible approach
  - guilhermefront 4y ago
    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.
zac23or 4y ago
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.
throwaway892238 4y ago
The worst thing during code reviews is people nit-picking my code or making comments that aren't important questions or mandatory changes. I didn't post this code review to get your two cents! I need to ship this code! Either give it a thumbs up, ask a relevant/important question with context, or ask me to change something that needs to be changed. Otherwise, shut up. 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.
- EdwardDiego 4y ago
  I left my last job because of the org issues that led to it being stupidly hard to land PRs, because of one project lead always demanding significant changes because he didn't like X, do Y, when both were valid approaches, he just preferred Y. 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.
- justahuman74 4y ago
  > I didn't post this code review to get your two cents! I'd say that you actually did, that's why it wasn't a yolo-merge
- j-bos 4y ago
  I dunno, while I often DM people about minor improvements rather than comment directly on their code. There is value to public comments that the entire team can see. And as a bonus comments can go in with an approval, signalling their optionality.
- justatdotin 4y ago
  there are two voices in my head. 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.
moomoo11 4y ago
Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. 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.
- doutunlined 4y ago
  Your comment implies juniors are only juniors for a temporary amount of time. I've worked with many SWEs (some with years of FAANG experience) who consistently code like juniors. They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. Consistently doing this over months erodes the quality of a codebase.
  - deleted 4y ago
    [deleted]
  - 4y ago
Ensorceled 4y ago
As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. 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.
- sidlls 4y ago
  "Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.
  - Ensorceled 4y ago
    Sure, we have code reviews for a reason, to make a better product. There are better places, and ways, to teach junior developers than long, painful code review processes.
    - lelandfe 4y ago
      Code review is the most common arena for getting feedback on my code. My team doesn't pair very often, though I think that's more effective. I don't believe I have other opportunities for it...?
      - astrange 4y ago
        If you're getting complete rearchitecture feedback in a code review, something's wrong because that's too late for it. Especially if it means rewriting later changes they haven't put up yet.
        dilyevsky 4y ago
twawaaay 4y ago
Code reviews are bad anyway. 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.
- de_keyboard 4y ago
  But then you are working on half as many things.
  - twawaaay 4y ago
    After quarter of century as a developer with 1/3rd of this time in pair programming teams I can say that's not true. 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.
  - globalise83 4y ago
    Often working on half as many things and executing them well is a good idea...
- jeffbee 4y ago
njharman 4y ago
The post isn't about not teaching during code reviews. It's about not doing it badly, duh? I'm actually shocked at his example. It's obviously bad practice (and just jerk/toxic behavior (don't fucking be coy, EXPLICIT > implicit)). It seems like strawman or cherry picked. I never experienced in 25 years. 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.
- deleted 4y ago
  [deleted]
rmk 4y ago
I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, "don't teach during code reviews" is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review, which is to catch bugs before they escape to the field, is lost as well. 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?
- Denzel 4y ago
  > Where else will people get habituated […]? 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.
  - rmk 4y ago
    Aside from the first, which is almost always out of date, the rest have been nonexistent at every company I have worked. And I do not even know what 'code labs' even means. My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.
    - Aeolun 4y ago
      I mean, at every company I worked at 100+ emloyees people were extremely enthusiastic about you scheduling a meeting for any reason.
    - Denzel 4y ago
      > almost always out of date Perfection need not be the enemy of good enough. Usually, whether the documentation is out of date or not, _some_ documentation is better than none. I've rarely come across an instance where documentation was actively harmful -- not to say those cases don't exist on the margins. > And I do not even know what 'code labs' even means. Code labs are self-guided lessons, like a tutorial, about the systems and technologies that your company is built on. They're called code labs because producing code/artifacts as part of your learning is one of the goals. For example, your company may have a custom enterprise installation process that's built upon Ansible, Terraform, AWS tooling, and a little proprietary work sprinkled on top. Some code labs may take you step-by-step through how to add a new service; configure, migrate, and wire up databases; etc. such that you'd have working code at the end of the code lab and new knowledge around the company's best practices. Similarly you can do this for the standard language at your company. Most companies accumulate some amount of proprietary helpers, libraries, or frameworks in their standard language. A code lab can walk you through how to write code in the way your company expects using the helpers, libraries, and frameworks that are common across your teams. > My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are. I have over a decade of similar experience across startups and large companies in NYC, Seattle, and SV. In the last 5 companies I've been at, all of them have had some combination of the above.
deleted 4y ago
[deleted]
drewcoo 4y ago
> " do you think we could rename this method to openRequest() or something along those lines?" > 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."
cphoover 4y ago
Disagree with "Don't teach during code reviews." I've found them to be invaluable opportunities for knowledge exchange
- lkbm 4y ago
  Yes, some of my most valuable programming education came from code reviews. 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.
userbinator 4y ago
to the yearslong experience I have improving and optimizing code review processes at Microsoft and beyond. 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.
deleted 4y ago
[deleted]
rogercafe 4y ago
I believe she had a good intention but her idea is not good at all. The Socratic Method is a fantastic way to trigger a health conversation about a topic without forcing a "senior" idea over a "junior". In my experience, asking someone to explain their intention behind a piece of code is a good way to: 1) Help the individual validate that the code is communicating their intentions 2) Discover if there is an different angle that the individual is not considering 3) Discover that the individual has actually produced a "good enough" quality and a risk/benefit analysis can be made with more information.
- spc476 4y ago
  Alan Kay spent the early part of his career explaining his ideas via The Socratic Method [1]. How, he spends his career yelling at us for failing to come to his conclusions. 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.
phailhaus 4y ago
> I’m not sure if I understand the whole idea but could you explain what this method does? 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.
- verzeichnis 4y ago
  The condescending part is that the reviewer understood how the method worked, but asked to have it explained to them again anyhow. That is dishonest. That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?
  - nonethewiser 4y ago
    I agree. And it can be good to rephrase your opinion into a question sometimes. You may think it should be done differently but it's good to know the reasoning behind things. But if it's the simple renaming example then by all means lead with the suggestion.
- ryanf 4y ago
  The question itself would be fine if the asker were actually interested in the answer. What's condescending is asking the question while already knowing the answer, with the unstated ulterior motive of getting the writer to change the method name.
- kazinator 4y ago
  What's wrong with it is that the reviewer knows well what the method does. The reviewer just wants the name to be changed to reflect that. He's just playing dumb, perpetrating the Socratic method to guide the reviewee to the conclusion/agreement that the method needs a different name, through the giant detour of having to explain what it does, when both parties already know that.
deleted 4y ago
[deleted]
deleted 4y ago
[deleted]
hakunin 4y ago
Here’s a list of practices I call mindful code reviews. You shouldn’t unnecessarily extend the loop, but you also shouldn’t rush or auto approve. https://max.engineer/mindful-code-reviews https://max.engineer/mindful-code-reviews
dilyevsky 4y ago
> And condescending feedback, master-apprentices relationships, and teachers-hats are contributing to such negative environments Wtf. I’m sure glad I cut my teeth before advice like this became widespread
lurchpop 4y ago
I have a manager who does the "teaching" thing poorly. If you're making a request in the form of a question, fine. But if you're trying to make me THINK, fuck off.
ahhssbhsshs 4y ago
There is a balance to be found between pointing out what is going to cause problems and what you find personally inelegant. What I find personally inelegant I typically leave as a suggestion, while approving the MR as a whole. The contributor - who has spent more time than me thinking about his code in the context of the issue - can finish things up on their own, taking my feedback into account where they see fit. This works well for most people. Very junior developers or those new to the team/tooling might need more explicit guidance on more mundane matters, so I will be more stringent on those matters, at least for the first few tickets. 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.
- schwartzworld 4y ago
  > what you find personally inelegant. 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.
  - djur 4y ago
    Pretty much any language has linters or even automatic formatters to handle stuff like this. If people are inclined to be opinionated on these subjects, better just to let a program handle it and leave it out of the reviews.
yazzku 4y ago
Clickbait title.
karmakaze 4y ago
It's saying don't be roundabout trying to play teacher in code reviews and other stuff. IMO code reviews often drag on longer than they ought to, so be direct and clear offering additional clarification az needed. Sometime even use Slack or video meet if that clears things up quicker and only record the outcome on the review. Respect people and their time. Also be open to sugestions and for reviewers not every suggestion is a must, make clear which ones are.
xyzelement 4y ago
On the flip side, try not to be the kind of person who can't be taught at every moment. 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.
faangiq 4y ago
Bad headline. If you’re not teaching what’s the point.
deleted 4y ago
[deleted]
SergeAx 4y ago
This is correct, except the ones between new team member and their mentor during onboarding. Those are specifically designed to give newbie all small ifs and buts we never put into written documentation. But at this time no one expects perfect velocity on those tasks.
mtVessel 4y ago
Using the Socratic method is being direct. It's directly showing the junior what their thought process should entail when they're writing code. I don't ask these questions because I get off on sounding superior, or want them to play guessing games. I ask these questions because they're the questions I ask myself when it's my work, and part of my job is demonstrating how they should approach their tasks.
l0b0 4y ago
> Today’s one is about how (not) to “teach” during code reviews. > 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.
awill88 4y ago
At the end of the day, since we’re talking about a human relationship between two individuals, it depends what you can and can’t do. In my opinion, do whatever you have to do to express your talent vicariously by completing your work to whatever standards matters most to the company you work for. Individuals can do whatever they want in code reviews. My advice is conduct yourself as the author says with humility and strict purpose. Teach, don’t teach, Who cares? Like, seriously. Jr devs, think for yourselves. So many snakes out here, watch your back. That’s the advice you should be getting sometimes.
worldsavior 4y ago
Agree with the approach, but it's a very aggressive one. It makes the submitter doubt himself and conclude conclusions, and it's not a fun experience.

196 points by trashymctrash 4y ago

> 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.

mytailorisrich 4y ago
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.
gtirloni 4y ago
> If you have a passive aggressive culture, something like this is a good fit. That would perpetuate the passive aggressive culture.
- chiefalchemist 4y ago
  That's exactly my point.

aquinas_ 4y ago

[flagged]

extr 4y ago

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.

sesuximo 4y ago

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!

tccole 4y ago
I think that goes to giving a why with a suggested change and giving a rough sketch.
rhizome 4y ago
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.
ketzo 4y ago
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?
- nonethewiser 4y ago
  What's the difference? Presumably teachers teach.
rictic 4y ago
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" Clear, actionable, refutable

HL33tibCe7 4y ago

The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.

coffeebeqn 4y ago
I just want people to be clear and concise. I hate the vague shit sandwich rhetorical questions. 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
- nonethewiser 4y ago
  I agree but I think honestly asking "why did you do it this way?" is important sometimes. The answer may completely change your suggestion. For the contrived example about renaming a method, no I dont think you should ask why they named it that way.
  - coffeebeqn 4y ago
    That is a straight question and definitely valid.
- conductr 4y ago
  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.

Someone 4y ago

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.

password11 4y ago
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?
- thinking4real 4y ago
  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?
  - wk_end 4y ago
    > 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...
    - nonethewiser 4y ago

ratherlongname 4y ago

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.

gloryjulio 4y ago

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

mephitix 4y ago

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…”

azov 4y ago

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?..

coldtea 4y ago
>like, you don't want to teach your partner how to dance while you're dancing Well, that's how you teach someone how to dance...
- throwawaymaths 4y ago
  Gp probably meant on the social dance floor which is generally considered to be taboo, and doing so is a mark of either a jerk or a noob.
  - pavlov 4y ago
    It really is surprisingly infuriating. 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.
    - deleted 4y ago
      [deleted]
    - throwawaymaths 4y ago
      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. So there are exceptions to the rule

revskill 4y ago

Why care much about "style" if you already know intention clearly ? 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.

yuliyp 4y ago
> 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.

languagehacker 4y ago

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.

tmsh 4y ago

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.

phreack 4y ago

Off-topic: 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.

hannes0 4y ago
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.
- napsterbr 4y ago
  Yeah, same here (iOS). I didn't bother trying to figure out where the "close button" was though.
- thinking4real 4y ago
  How else are they going to get spam email lists based off poorly advised articles?
  - rhizome 4y ago
    Flame-bait gets good traffic, can't let that engagement go to waste!
amatecha 4y ago
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.

koof 4y ago

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.)

kdazzle 4y ago
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?”
- lmz 4y ago
  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.
  - doutunlined 4y ago
    Your code reviews are doomed regardless if the feedback cycle is 1 day.

why-el 4y ago

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.

johtso 4y ago
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.

sidlls 4y ago

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.

deleted 4y ago
[deleted]
bsder 4y ago
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.
- lifeisstillgood 4y ago
  This. 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
- dan-robertson 4y ago
  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.

cat_plus_plus 4y ago

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.

amatecha 4y ago
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.
tyingq 4y ago
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.
- nonethewiser 4y ago
  That's why _honestly_ asking why they did something a certain way first can be a good idea.

jeffbee 4y ago

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.

foota 4y ago

I would suggest reading the article before commenting. The suggestion is to not make vague statements and instead say what you mean, not that code reviews shouldn't be educational.

Scubabear68 4y ago

This post is too one-size-fits-all. 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.

politelemon 4y ago

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.

CrimpCity 4y ago

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.

gitgud 4y ago

> I’m not sure if I understand the whole idea but could you explain what this method does? 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...

thinking4real 4y ago
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 This is such a terrible approach
- guilhermefront 4y ago
  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.

zac23or 4y ago

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.

throwaway892238 4y ago

The worst thing during code reviews is people nit-picking my code or making comments that aren't important questions or mandatory changes. I didn't post this code review to get your two cents! I need to ship this code! Either give it a thumbs up, ask a relevant/important question with context, or ask me to change something that needs to be changed. Otherwise, shut up. 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.

EdwardDiego 4y ago
I left my last job because of the org issues that led to it being stupidly hard to land PRs, because of one project lead always demanding significant changes because he didn't like X, do Y, when both were valid approaches, he just preferred Y. 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.
justahuman74 4y ago
> I didn't post this code review to get your two cents! I'd say that you actually did, that's why it wasn't a yolo-merge
j-bos 4y ago
I dunno, while I often DM people about minor improvements rather than comment directly on their code. There is value to public comments that the entire team can see. And as a bonus comments can go in with an approval, signalling their optionality.
justatdotin 4y ago
there are two voices in my head. 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.

moomoo11 4y ago

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. 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.

doutunlined 4y ago
Your comment implies juniors are only juniors for a temporary amount of time. I've worked with many SWEs (some with years of FAANG experience) who consistently code like juniors. They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. Consistently doing this over months erodes the quality of a codebase.
- deleted 4y ago
  [deleted]
- 4y ago

Ensorceled 4y ago

As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. 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.

sidlls 4y ago
"Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.
- Ensorceled 4y ago
  Sure, we have code reviews for a reason, to make a better product. There are better places, and ways, to teach junior developers than long, painful code review processes.
  - lelandfe 4y ago
    Code review is the most common arena for getting feedback on my code. My team doesn't pair very often, though I think that's more effective. I don't believe I have other opportunities for it...?
    - astrange 4y ago
      If you're getting complete rearchitecture feedback in a code review, something's wrong because that's too late for it. Especially if it means rewriting later changes they haven't put up yet.
      - dilyevsky 4y ago

twawaaay 4y ago

Code reviews are bad anyway. 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.

de_keyboard 4y ago
But then you are working on half as many things.
- twawaaay 4y ago
  After quarter of century as a developer with 1/3rd of this time in pair programming teams I can say that's not true. 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.
- globalise83 4y ago
  Often working on half as many things and executing them well is a good idea...
jeffbee 4y ago

njharman 4y ago

The post isn't about not teaching during code reviews. It's about not doing it badly, duh? I'm actually shocked at his example. It's obviously bad practice (and just jerk/toxic behavior (don't fucking be coy, EXPLICIT > implicit)). It seems like strawman or cherry picked. I never experienced in 25 years. 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.

deleted 4y ago
[deleted]

rmk 4y ago

I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, "don't teach during code reviews" is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review, which is to catch bugs before they escape to the field, is lost as well. 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?

Denzel 4y ago
> Where else will people get habituated […]? 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.
- rmk 4y ago
  Aside from the first, which is almost always out of date, the rest have been nonexistent at every company I have worked. And I do not even know what 'code labs' even means. My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.
  - Aeolun 4y ago
    I mean, at every company I worked at 100+ emloyees people were extremely enthusiastic about you scheduling a meeting for any reason.
  - Denzel 4y ago
    > almost always out of date Perfection need not be the enemy of good enough. Usually, whether the documentation is out of date or not, _some_ documentation is better than none. I've rarely come across an instance where documentation was actively harmful -- not to say those cases don't exist on the margins. > And I do not even know what 'code labs' even means. Code labs are self-guided lessons, like a tutorial, about the systems and technologies that your company is built on. They're called code labs because producing code/artifacts as part of your learning is one of the goals. For example, your company may have a custom enterprise installation process that's built upon Ansible, Terraform, AWS tooling, and a little proprietary work sprinkled on top. Some code labs may take you step-by-step through how to add a new service; configure, migrate, and wire up databases; etc. such that you'd have working code at the end of the code lab and new knowledge around the company's best practices. Similarly you can do this for the standard language at your company. Most companies accumulate some amount of proprietary helpers, libraries, or frameworks in their standard language. A code lab can walk you through how to write code in the way your company expects using the helpers, libraries, and frameworks that are common across your teams. > My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are. I have over a decade of similar experience across startups and large companies in NYC, Seattle, and SV. In the last 5 companies I've been at, all of them have had some combination of the above.

deleted 4y ago

[deleted]

drewcoo 4y ago

> " do you think we could rename this method to openRequest() or something along those lines?" > 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."

cphoover 4y ago

Disagree with "Don't teach during code reviews." I've found them to be invaluable opportunities for knowledge exchange

lkbm 4y ago
Yes, some of my most valuable programming education came from code reviews. 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.

userbinator 4y ago

to the yearslong experience I have improving and optimizing code review processes at Microsoft and beyond. 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.

deleted 4y ago

[deleted]

rogercafe 4y ago

I believe she had a good intention but her idea is not good at all. The Socratic Method is a fantastic way to trigger a health conversation about a topic without forcing a "senior" idea over a "junior". In my experience, asking someone to explain their intention behind a piece of code is a good way to: 1) Help the individual validate that the code is communicating their intentions 2) Discover if there is an different angle that the individual is not considering 3) Discover that the individual has actually produced a "good enough" quality and a risk/benefit analysis can be made with more information.

spc476 4y ago
Alan Kay spent the early part of his career explaining his ideas via The Socratic Method [1]. How, he spends his career yelling at us for failing to come to his conclusions. 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.

phailhaus 4y ago

> I’m not sure if I understand the whole idea but could you explain what this method does? 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.

verzeichnis 4y ago
The condescending part is that the reviewer understood how the method worked, but asked to have it explained to them again anyhow. That is dishonest. That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?
- nonethewiser 4y ago
  I agree. And it can be good to rephrase your opinion into a question sometimes. You may think it should be done differently but it's good to know the reasoning behind things. But if it's the simple renaming example then by all means lead with the suggestion.
ryanf 4y ago
The question itself would be fine if the asker were actually interested in the answer. What's condescending is asking the question while already knowing the answer, with the unstated ulterior motive of getting the writer to change the method name.
kazinator 4y ago
What's wrong with it is that the reviewer knows well what the method does. The reviewer just wants the name to be changed to reflect that. He's just playing dumb, perpetrating the Socratic method to guide the reviewee to the conclusion/agreement that the method needs a different name, through the giant detour of having to explain what it does, when both parties already know that.

deleted 4y ago

[deleted]

deleted 4y ago

[deleted]

hakunin 4y ago

Here’s a list of practices I call mindful code reviews. You shouldn’t unnecessarily extend the loop, but you also shouldn’t rush or auto approve. https://max.engineer/mindful-code-reviews https://max.engineer/mindful-code-reviews

dilyevsky 4y ago

> And condescending feedback, master-apprentices relationships, and teachers-hats are contributing to such negative environments Wtf. I’m sure glad I cut my teeth before advice like this became widespread

lurchpop 4y ago

I have a manager who does the "teaching" thing poorly. If you're making a request in the form of a question, fine. But if you're trying to make me THINK, fuck off.

ahhssbhsshs 4y ago

There is a balance to be found between pointing out what is going to cause problems and what you find personally inelegant. What I find personally inelegant I typically leave as a suggestion, while approving the MR as a whole. The contributor - who has spent more time than me thinking about his code in the context of the issue - can finish things up on their own, taking my feedback into account where they see fit. This works well for most people. Very junior developers or those new to the team/tooling might need more explicit guidance on more mundane matters, so I will be more stringent on those matters, at least for the first few tickets. 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.

schwartzworld 4y ago
> what you find personally inelegant. 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.
- djur 4y ago
  Pretty much any language has linters or even automatic formatters to handle stuff like this. If people are inclined to be opinionated on these subjects, better just to let a program handle it and leave it out of the reviews.

yazzku 4y ago

Clickbait title.

karmakaze 4y ago

It's saying don't be roundabout trying to play teacher in code reviews and other stuff. IMO code reviews often drag on longer than they ought to, so be direct and clear offering additional clarification az needed. Sometime even use Slack or video meet if that clears things up quicker and only record the outcome on the review. Respect people and their time. Also be open to sugestions and for reviewers not every suggestion is a must, make clear which ones are.

xyzelement 4y ago

On the flip side, try not to be the kind of person who can't be taught at every moment. 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.

faangiq 4y ago

Bad headline. If you’re not teaching what’s the point.

deleted 4y ago

[deleted]

SergeAx 4y ago

This is correct, except the ones between new team member and their mentor during onboarding. Those are specifically designed to give newbie all small ifs and buts we never put into written documentation. But at this time no one expects perfect velocity on those tasks.

mtVessel 4y ago

Using the Socratic method is being direct. It's directly showing the junior what their thought process should entail when they're writing code. I don't ask these questions because I get off on sounding superior, or want them to play guessing games. I ask these questions because they're the questions I ask myself when it's my work, and part of my job is demonstrating how they should approach their tasks.

l0b0 4y ago

> Today’s one is about how (not) to “teach” during code reviews. > 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.

awill88 4y ago

At the end of the day, since we’re talking about a human relationship between two individuals, it depends what you can and can’t do. In my opinion, do whatever you have to do to express your talent vicariously by completing your work to whatever standards matters most to the company you work for. Individuals can do whatever they want in code reviews. My advice is conduct yourself as the author says with humility and strict purpose. Teach, don’t teach, Who cares? Like, seriously. Jr devs, think for yourselves. So many snakes out here, watch your back. That’s the advice you should be getting sometimes.

worldsavior 4y ago

Agree with the approach, but it's a very aggressive one. It makes the submitter doubt himself and conclude conclusions, and it's not a fun experience.