[deleted]
Don’t teach during code reviews
261–270 of 304 posts
Re: Don’t teach during code reviews
#262> 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 w…
So according to you, the following would be better?
> This method name is a bit too vague, what is it trying to do?
Re: Don’t teach during code reviews
#263This 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.
Re: Don’t teach during code reviews
#264Earlier quoted context omitted.
> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback. I’m looking for basics. Will it work? is the code clear? is it commented/documented…
To be fair, some people are just pedantic. Just because you are the reviewer doesn’t mean things have to be done exactly the way you say. If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated? I aim for one or at most two rounds of review, because anything else is just depressing.
Ehhh far from perfection, more like “please use the native feature that handles this in 2 LOC instead of shelling out to an external 100 line script”
Re: Don’t teach during code reviews
#265Using 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.
Re: Don’t teach during code reviews
#266The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.
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
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.
Re: Don’t teach during code reviews
#267Earlier quoted context omitted.
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…
I actually like the “one interface with one implementation” as it makes for less surprises.
With an interface I know that only that method can be called. If a class is passed in i have no idea what methods going to be used. It also gives me a better feeling that the coder has in mind what they need a parameter to do without worrying that they are going to rely on a couple of othogonal methods to be called.
Re: Don’t teach during code reviews
#268Earlier quoted context omitted.
> 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. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit s…
This possibly be true in some case, but I find it to mostly be wrong. The best time to fix up some code is when you are already modifying it, because that’s when you are already fully in context to understand the effects. Going back later to clean stuff up is much less likely, because you have to pay that marginal comprehension cost again and again for each issue you fix (and are less likely to even notice the proble…
> This possibly be true in some case, but I find it to mostly be wrong.
Yes. That’s why it’s an important skill for an engineer to know when to make that tradeoff.
Sounds like you’re agreeing with the grandparent comment?
Re: Don’t teach during code reviews
#269Earlier quoted context omitted.
I'm gonna call bullshit on all of this. The time tax on not leaking layers in an application that doesn't already leak layers is nearly zero. The problem is people don't want to expend the very minor extra amount of effort. So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line. After a…
In my experience, codebases never maintain their level of quality. Unless you are taking deliberate action to improve the codebase with every commit, it's actively getting worse like you described above. The interesting thing is that this isn't a technical problem, it's a human problem. Most folks want to clock in, do their job and clock out, and going the extra mile to improve things isn't worth the effort. I think…
And that's why you don't let any bullshit in, exercise eternal vigilance and rely on static analysis tools to stem the turd wave in an automatic fashion as much as you can.
The solution is not to accept "it is what it is".
Re: Don’t teach during code reviews
#270Earlier quoted context omitted.
> 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. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit s…
I'm gonna call bullshit on all of this. The time tax on not leaking layers in an application that doesn't already leak layers is nearly zero. The problem is people don't want to expend the very minor extra amount of effort. So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line. After a…
I mostly agree, BUT, it's very possible to end up with a situation where your current layers, even if they don't leak, are ill-designed for the feature you're working on. It can be a lot of work at that point to keep it non-leaking. You should still do it, but it won't be "nearly zero" cost.