[HN Gopher] Don't teach during code reviews
___________________________________________________________________
Don't teach during code reviews
Author : trashymctrash
Score : 121 points
Date : 2023-02-05 18:55 UTC (4 hours ago)
(HTM) web link (www.michaelagreiler.com)
(TXT) w3m dump (www.michaelagreiler.com)
| chiefalchemist wrote:
| > 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 wrote:
| 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 wrote:
| _> If you have a passive aggressive culture, something like
| this is a good fit._
|
| That would perpetuate the passive aggressive culture.
| foota wrote:
| 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.
| sidlls wrote:
| 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.
| aflag wrote:
| 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.
| [deleted]
| [deleted]
| hgsgm wrote:
| "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.
| bsder wrote:
| 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.
| dan-robertson wrote:
| 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.
| SilasX wrote:
| >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.)
| sidlls wrote:
| 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.
| actually_a_dog wrote:
| 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?
| lifeisstillgood wrote:
| 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
| impalallama wrote:
| Socrates was executed for the Socratic method
| throwaway892238 wrote:
| Ask them if they want a lesson first. Not every junior wants
| their superiors lecturing them.
| sidlls wrote:
| A junior engineer who doesn't want to learn from a senior
| engineer is an engineer setting himself up for a failed
| career.
| tremon wrote:
| 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.
| thinking4real wrote:
| 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
| goostavos wrote:
| 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".
|
| to: "what is the nature of being?"
| mentat wrote:
| 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.
| PragmaticPulp wrote:
| > 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.
| NikolaNovak wrote:
| 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 :-)
| PragmaticPulp wrote:
| 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.
| onlypositive wrote:
| +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.
| [deleted]
| dgb23 wrote:
| 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.
| [deleted]
| azangru wrote:
| > 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?
| BlackFly wrote:
| 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.
| photochemsyn wrote:
| 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.
| simplotek wrote:
| > 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.
| m463 wrote:
| what some people don't get is that they're gatekeeper and have
| to balance that out.
| zac23or wrote:
| 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.
| mephitix wrote:
| 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..."
| [deleted]
| revskill wrote:
| 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.
| twawaaay wrote:
| 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 wrote:
| But then you are working on half as many things.
| twawaaay wrote:
| 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 wrote:
| Often working on half as many things and executing them well
| is a good idea...
| jeffbee wrote:
| Anyone who resents code review isn't fit for this business.
| This is exactly why information given by interviewers to hiring
| committees focuses on the nature of the back-and-forth exchange
| during the interview, and decidedly not on whether the
| candidate completed the exercise. The most important feature of
| a software engineer is how well they fit into organizations.
|
| Same above goes for anyone who believes they have "finished the
| work" before getting any reviews.
| twawaaay wrote:
| Ever joined a team and needed to get results from them?
| Usually firing every single person and starting to hire for
| your high standards is not an option.
|
| There is your idealised view of how software development
| should work and then there is the real world. I prefer to
| stay grounded in the real world and figure out actual ways to
| help people succeed rather than telling them how they should
| be performing and firing if they can't meet my standard.
|
| Whatever you think about what people "should be feeling", the
| truth is when they made their last commit almost everybody
| feels they finished their work and when it is thrown back to
| them they do not welcome it. I have never seen a hiring
| process able to only pass people who are happy to get review
| comments requiring them to review large part of their work.
| drewcoo wrote:
| > " 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."
| cat_plus_plus wrote:
| 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.
| tyingq wrote:
| 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.
| amatecha wrote:
| 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.
| userbinator wrote:
| _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.
| languagehacker wrote:
| 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.
| extr wrote:
| 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 wrote:
| 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 wrote:
| I think that goes to giving a why with a suggested change and
| giving a rough sketch.
| ketzo wrote:
| 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?
| rhizome wrote:
| 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.
| azov wrote:
| 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 wrote:
| > _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 wrote:
| 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 wrote:
| 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]
| [deleted]
| Swizec wrote:
| > 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.
| wizzwizz4 wrote:
| > _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.
| photochemsyn wrote:
| Concisely, teacher-student relationships should be formally
| agreed upon in advance of the actual interaction.
| Scubabear68 wrote:
| 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.
| [deleted]
| throwaway892238 wrote:
| 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.
| justatdotin wrote:
| 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.
| endtime wrote:
| I have a very different perspective. I am at that point in my
| career when I am lucky if I get to code for a day or two out of
| the week, and I recently wrote my first PR in a new language.
| My reviewer knew that language well and his PR was full of
| nitpicks, and it was great. I learned a few things, and it
| helped me write cleaner code the next time around.
| justahuman74 wrote:
| > 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
| EdwardDiego wrote:
| 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.
| j-bos wrote:
| 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.
| politelemon wrote:
| 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.
| moomoo11 wrote:
| 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 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 wrote:
| 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]
| Zagill wrote:
| I've always had a pretty positive attitude towards code reviews
| (still a junior dev), because I know there's probably something
| I missed or a technique I'm not familiar with or some language
| quirk I didn't know about. If I submit a PR with a bunch of new
| functionality, I'm going to be way more concerned if I _don 't_
| get a handful of comments on it than if I do.
| alwaysbeconsing wrote:
| Frankly I think everyone should have this attitude, seniors
| as well. (I try to) I am experienced but I know I write bugs
| and code that may be not as clear as it could be. We all do:
| we're only human.
|
| Also depending on the codebase (and language to some degree),
| if someone senior writes a lot of overcomplex or abstract
| code that the rest of the team can't understand/maintain,
| that's just as much a problem as anything else.
| [deleted]
| CrimpCity wrote:
| 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.
| tmsh wrote:
| 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.
| Ensorceled wrote:
| 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 gauling 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.
| bt4u wrote:
| You have to fight to produce quality and you have to do it
| continuously. You cannot just merge in "working(tm) but not
| using best practices" and then turn on the quality-switch
| months or years later.
|
| Your attitude is creating a culture where nobody cares and it's
| why many good engineers end up hating their job.
| ferdowsi wrote:
| The vast majority of questions around quality that end up
| surfacing in code reviews can be much more easily settled by
| commonly shared tooling that enforces organizational style
| and security practices. Leaving "best practice" up to the
| code review stage just ends up allowing senior engineers to
| demand code that they themselves would write, not what's best
| for an organization.
| Ensorceled wrote:
| Wow. You sure are extrapolating a lot about me from a simple
| comment.
|
| While we're extrapolating, are you the type that makes junior
| developers rewrite their loops as list comprehensions or the
| type that makes them rewrite their list comprehensions as
| loops? Because I've seen both in code reviews.
| doutunlined wrote:
| Why would you rewrite them as loops?
| Ensorceled wrote:
| "It's much easier to read a loop than a list
| comprehension, please rewrite all the list comprehensions
| as loops."
|
| I squashed that.
| [deleted]
| lmm wrote:
| > While we're extrapolating, are you the type that makes
| junior developers rewrite their loops as list
| comprehensions or the type that makes them rewrite their
| list comprehensions as loops? Because I've seen both in
| code reviews.
|
| One of them is right, because a codebase that consistently
| uses one or the other is better than a codebase that mixes
| the two. If the team hasn't made and communicated a
| decision then that sounds like a management failing.
|
| (list comprehensions are the actual right thing to use, for
| the record, but that's far less important than having a
| standard and sticking to it)
| mjlawson wrote:
| I think that these kind of reviews can potentially be
| squashed by building consensus around what kind of feedback
| is right for PRs, what conventions you agree upon using,
| etc. Far too often, these kind of "best practices" only
| exist in the senior's head (if they're even truly best
| practices at all), so it becomes a very frustrating moving
| target for a junior.
|
| I agree you should push back against this as a manager, but
| it can be hard to do so tactfully from my experiences. You
| either have to say, "no," or engage in protracted debates
| on subjective ideas around readability and maintainability.
| Ensorceled wrote:
| > I agree you should push back against this as a manager,
| but it can be hard to do so tactfully from my
| experiences.
|
| Just review this thread ... it can be utterly painful.
| There are at least as many senior devs who don't like to
| take feedback on how to deliver feedback as junior devs
| who don't like to take feedback.
| doutunlined wrote:
| "It works" is perhaps the lowest bar you can possibly have for
| merging code.
| BlackjackCF wrote:
| I've always thought if it's a "teaching moment" that it makes
| more sense to just pair with the engineer and go through it
| together. Otherwise it's a really long turnaround to go back
| and forth in PR comments that just frustrates all parties.
| makeitdouble wrote:
| Depending on what you're pointing at, it might be better to
| let the other party take whatever time they need to digest
| your comments and adjust (including comming up with counter
| points)
|
| For instance if you're telling them about a behavior defined
| in a RFC, it can have better long lasting impact if they get
| the time to read and understand it on their own, than if
| you're over their shoulder shoving info at them.
|
| Basically, live teaching requires you to be good at reading
| the other person's state and providing the relevant info in a
| digestible way, while they live code. It can work, but that's
| a high bar to pass.
| baby wrote:
| I agree to some extent. Personally, I do spend the time to
| teach in PRs but also approve the PR to unblock unless there's
| much needed changes. It unblocks people, and you quickly see if
| they cared about your comments if they follow up with another
| PR addressing them.
| bigyikes wrote:
| Yes, this is how you have your cake and eat it.
|
| It works in reverse too, when you're the one being reviewed
| and you receive feedback you disagree with.
|
| Instead of dying on the hill and delaying your merge, just
| implement the feedback _and then_ argue. You can make your
| point while still accommodating your teammate and moving
| things along.
|
| (Obviously, this practice shouldn't apply to feedback which
| is truly bad)
| rmk wrote:
| This is more prevalent than one thinks, particularly at large
| companies where senior developers will ruthlessly put
| roadblocks, either as a way of carrying out group politics, or
| keeping control over things where the risk of a new idea or
| paradigm exceeds the possible downsides, however minor, of
| rocking the boat.
|
| However, if this is in a small team, then it's a management
| problem that needs to be solved by the people manager. Either
| the hiring process is not eliminating bad performers or people
| with bad attitudes, or team dynamics have deteriorated
| considerably.
|
| From the senior's viewpoint, though, if they are going to be
| held solely responsible for janitorial work to avert juniors'
| substandard work blowing up down the road, or picking up the
| pieces when that does happen, then it makes perfect sense for
| the senior to put up roadblocks, junior dev's career be damned.
| Again, this is likely a management problem where people are not
| held accountable and made to maintain systems they come up with
| (tenure is so short, especially among junior people, that this
| is likely to be a systemic problem).
| justahuman74 wrote:
| On the other hand, some people actually write stuff that isn't
| great but visually seems ok. You have the senior engineer to
| say 'no lets not create a mess'
| Ensorceled wrote:
| Yes. Why do you think I have code reviews if not to stop
| messes from getting made?
|
| I'm not talking about that.
| makeitdouble wrote:
| > it's especially gauling if the original PR was working,
| reasonably well written, no major flaws [...] wasn't "best
| practices".
|
| You see a clear dichotomy between well written code and what
| your senior engieers see as enforceable best practices.
|
| I'd guess you also see half of the time your team spends on
| coding as some useless nitpicking you're letting them get away
| with by sheer generousity or bcause you don't have a
| choice...From the looks of it that can't be a healthy team
| dynamic
| Ensorceled wrote:
| > You see a clear dichotomy between well written code and
| what your senior engieers see as enforceable best practices.
|
| No, in general, I agree with code review comments my senior
| developers make. I'm talking about the difference between
| "best practices" and actual best practices or in the
| difference between "this works, but next time, a better
| architecture would be x, let's refactor next time we come
| back to this" and "I'm blowing up the sprint"
| azov wrote:
| Well, here is one way to read your comment:
|
| "A feature not making it into a sprint" is your problem. You
| will get in trouble for that, it will make you look bad in
| front of your boss, etc. "Not following best practices" is not
| your problem. If the code turns into a mess, it's not you who
| will have to deal with the consequences, at least not you
| directly. As a manager you will demand that your senior
| developers fix that (while still shipping features, of course).
| After all, that's what expected of senior developers.
|
| Of course I don't know if that's even remotely close to your
| reasoning. I don't know you or your project or your priorities,
| I have no idea what "best practices" we're talking about and
| how reasonable it is to follow or not follow them in a given
| situation. I might be totally wrong, you may have all the right
| reasons to be mad about those reviews. But the comment makes it
| sound like your senior engineers are idiots who block PRs for
| no reason, and that can't be good for your team.
| Ensorceled wrote:
| > But the comment makes it sound like your senior engineers
| are idiots who block PRs for no reason, and that can't be
| good for your team.
|
| Ah, I see, you think this is a general problem I have. No, in
| general my senior engineers are excellent and all the current
| ones I work with are.
|
| I've seen enough of the other, especially while contracting.
| ferdowsi wrote:
| Agreed. Too many "senior engineers" fail to recognize when they
| are simply venting their territorialism by nitpicking in code
| reviews. The role of a good manager is to restrain these
| tendencies.
| peteradio wrote:
| If you don't want to see that drawn out at code review time,
| then have the juniors consult with the seniors prior to that
| stage. If you don't want seniors holding juniors to standards
| then do away with the junior/senior title separation because it
| might be meaningless to you.
| Ensorceled wrote:
| If I thought the junior was not up to the task I assigned
| them, I _would_ have had them consult with a senior developer
| during the task.
|
| I'm literally talking about the case where a senior developer
| decides that this task was not implemented to their personal
| standards AND makes it a "teaching moment" that means it
| doesn't get delivered this sprint.
|
| Saying "this can't ship without significant rework" is
| something that needs to get escalated, not ground out in a
| code review.
| kissgyorgy wrote:
| People without technical background shouldn't be allowed to be
| managers of highly technical teams.
| l33t233372 wrote:
| I don't see what here implies the grandparent commenter does
| not have a technical background.
| Ensorceled wrote:
| I agree. Wait, do you think I'm non-technical?
| razemio wrote:
| At least I think so because of your previous comment. Since
| I can only take this as context their might be a
| misunderstanding. Still I would like to point out a few
| thinks, I did not like about your comment.
|
| In my opinion, you are looking at the problem from the
| wrong angle. If code gets worse (non working) after a code
| review, the reviewer made some serious mistakes and/or was
| not guiding his colleague to the correct solution like it
| should be. Also if working code gets rejected because of
| bad practice, you should not be mad about it but be
| thankful that someone caught that before release. If your
| only measurement of code quality, is that something is
| working, I would consider that a dangerous practice.
| Ensorceled wrote:
| > If your only measurement of code quality, is that
| something is working, I would consider that a dangerous
| practice.
|
| I'm really confused as to why there are so many people
| assuming I don't give a shit about quality when I have
| CODE REVIEWS.
|
| Why are you assuming I want them all to just be rubber
| stamps?
|
| And what God Complex do you have to have that you assume
| the "senior developer" is ALWAYS in the right, and the
| junior and the "non-technical" manager just don't respect
| "quality" at all?
| strix_varius wrote:
| Not OP, but your final comment confuses me -
|
| Given a senior developer, a junior developer, and a non-
| technical manager, in the context of a code review, the
| vast majority of the time you should absolutely listen to
| the senior developer.
|
| If that's not true in your organization, then hiring,
| leveling, and leadership should all be questioned.
| Hermitian909 wrote:
| (I'm Not OP) - At the end of the day we're all trying to
| strike a balance between shipping fast and keeping
| technical debt down. Asking juniors to only ship code
| that's good as what a senior would write is very rarely
| the right balance point. Senior engineers must learn to
| understand when code is "good enough". If you're a senior
| and you haven't developed this skill you're bringing your
| team down.
|
| Concretely, a heuristic I rely on is to understand
| whether the sub-par code being added "infects" the code
| base e.g. it changes an important interface, it adds a
| poorly designed interface other code will rely on etc.
| These are the places its important to put your foot down.
| Conversely, if the internals of a one-off function or
| class could be a little cleaner... eh, ship it.
| sidlls wrote:
| "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 wrote:
| 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.
| sidlls wrote:
| I just strongly disagree with this. The context of a code
| review is the perfect opportunity to teach, much like a
| technical design review is for system designs, and having
| them sit as an observer during an incident response is for
| triage/debugging a live system under stress. There are no
| better settings for teaching these skills to juniors. And
| you should really let your senior engineers take advantage
| of these times to impart their wisdom. It will help them
| grow junior engineers into high quality seniors that will
| help make your team's output better and faster in the long
| run.
| strix_varius wrote:
| > There are better places, and ways, to teach junior
| developers than long, painful code review processes.
|
| If I had to choose the best single way to grow as a
| software engineer, it would be through code reviews: both
| giving and receiving.
|
| That said, "long, painful" code reviews is a red flag for
| your team. Code reviews should have one of four outcomes:
|
| 1. Lgtm.
|
| 2. Approved. Left some suggestions for your consideration.
|
| 3. Specific changes requested.
|
| 4. This whole approach needs to be re-evaluated, let's
| talk.
|
| _None_ of those should be long or painful. It sounds like
| your team might be doing #4 in the PR itself when really
| that's an exceptional case that should make it to a
| usually-synchronous discussion.
| penjelly wrote:
| i disagree, it is the place. The work shouldnt be at risk
| of missing release to begin with, timelines should not
| _assume_ a senior dev works the issue.
| fatjokes wrote:
| Like what? What's that Confucius saying: "I hear and I
| forget, I see and I remember, I do and I understand"?
| Nothing beats teaching using a real example that the
| parties are hands on with.
|
| I get what you're saying. Delaying a milestone to have a
| long convo in a CR is not good prioritization. But as the
| _technical_ manager it 's on you to budget more time for
| junior devs to land their changes.
| lelandfe wrote:
| 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...?
| HL33tibCe7 wrote:
| The proposed approach to code review in the mentioned medium
| article makes my skin crawl. So patronising and passive-
| aggressive.
| draw_down wrote:
| [dead]
| silveroriole wrote:
| 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.
| [deleted]
| coffeebeqn wrote:
| 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
| aquinas_ wrote:
| [flagged]
| Someone wrote:
| 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 wrote:
| 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?
| watwut wrote:
| 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.
|
| No good teacher does that.
| thinking4real wrote:
| 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 wrote:
| > 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...
| gitgud wrote:
| _> 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 wrote:
| 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 wrote:
| 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.
| cphoover wrote:
| Disagree with "Don't teach during code reviews."
|
| I've found them to be invaluable opportunities for knowledge
| exchange
| phreack wrote:
| 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 wrote:
| 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 wrote:
| Yeah, same here (iOS). I didn't bother trying to figure out
| where the "close button" was though.
| thinking4real wrote:
| How else are they going to get spam email lists based off
| poorly advised articles?
| rhizome wrote:
| Flame-bait gets good traffic, can't let that engagement go
| to waste!
| amatecha wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| Your code reviews are doomed regardless if the feedback
| cycle is 1 day.
| rogercafe wrote:
| 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.
| njharman wrote:
| 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]
| ratherlongname wrote:
| 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.
| jeffbee wrote:
| 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.
| gloryjulio wrote:
| 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
| why-el wrote:
| 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 wrote:
| 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.
| rmk wrote:
| 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?
| phailhaus wrote:
| > 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.
___________________________________________________________________
(page generated 2023-02-05 23:00 UTC)