Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Don’t teach during code reviews

michaelagreiler.com

Post reply on HN
Print view View original

191–200 of 304 posts

Re: Don’t teach during code reviews

#191

Post by ordu »

phailhaus wrote: post #148

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

> What's wrong with this?

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

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

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

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

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

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

Re: Don’t teach during code reviews

#192

Post by simplotek »

jvans wrote: post #166
alwaysbeconsing wrote: post #147

Earlier quoted context omitted.

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…

> if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people her…

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

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

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

Re: Don’t teach during code reviews

#194

Post by radnor »

azov wrote: post #163
Ensorceled wrote: post #123

Earlier quoted context omitted.

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

> 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. Having code reviews is an incredibly low bar. The very fact that you offer this as an evidence might be the reason people make those assumptions.

Is it? Their comment might come from having worked at companies where code reviews aren't even a thing, let alone source control. It's crazy out there, especially if you're nowhere near Silicon Valley.

Re: Don’t teach during code reviews

#195

Post by strix_varius »

logbiscuitswave wrote: post #187
Ensorceled wrote: post #152

Earlier quoted context omitted.

> 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" an…

“this works, but next time, a better architecture would be x, let's refactor next time we come back to this” In my experience there is rarely ever a “next time” and “fix it later” becomes “fix it never”. It’s always cheapest to fix worst practices up front rather than letting them metastasize into a huge pile of technical debt later. It’s also rare to find anybody interested in or willing to go back and fix old stuff…

Exactly - this, coupled with phrases like "blowing up the sprint," suggests that nobody on this team is likely to trust that the offer of "fixing it later, next time" will be held to.

Re: Don’t teach during code reviews

#196

Post by jvans »

moomoo11 wrote: post #174
jvans wrote: post #166

Earlier quoted context omitted.

> if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people her…

Is the abstraction so that they can easily swap out pieces of the system without a single headache, or write effective tests, or to make it easy to operate in a soup of services? Because then it makes complete sense. Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.

There's a long list of things that I think are abused in an effort to save on some vaguely defined future cost that never seems to materialize. Some things off the top of my head:

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

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

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

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

Re: Don’t teach during code reviews

#197

Post by ahhssbhsshs »

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.

Re: Don’t teach during code reviews

#198

Post by geraldwhen »

projectazorian wrote: post #164
Ensorceled wrote: post #152

Earlier quoted context omitted.

> 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" an…

Comments such as “blowing up the sprint” suggest a habit of catastrophizing because an artificial deadline wasn’t met, that’s not going to serve you or your teams well in the long run. When I hear comments like this from managers it’s generally a sign that I need to start planning an exit strategy. edit: Also, “my senior developers?” They’re human beings, not your property, dude

Honestly it sounds like a terrible place to work. There’s artificial deadlines with no slack for error, and Junior devs are unable to learn to allow them to be better in the future.

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

Re: Don’t teach during code reviews

#199

Post by quadrifoliate »

Ensorceled wrote: post #123
razemio wrote: post #110

Earlier quoted context omitted.

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

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

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

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

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

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

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

Re: Don’t teach during code reviews

#200

Post by emeraldd »

sesuximo wrote: post #6

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!

Agreed, this isn't about teaching. It's about people who stroke their own egos when they should really be instructing. The example comment doesn't tell you anything useful except that the commenter might not understand what the author is trying to do. That has no call to action, no indication of a problem, and basically nothing useful to the author. It is a complete waste of time ...
Post reply on HN