Earlier quoted context omitted.
Every line of code must be code reviewed before it goes to stage/prod
… how else do people operate? Complete yolo?
Don’t teach during code reviews
281–290 of 304 posts
Re: Don’t teach during code reviews
#282Earlier 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.
Re: Don’t teach during code reviews
#283Earlier 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…
The time tax is paid by not delivering a feature. Architecture is supposed to be there to help you, not fail deadlines.
Also, it seems you completely failed to read or get what I wrote. My point was that if a PR delivers a feature without defects then your team is far better off if you accept the PR and refactor later when you can spare the time then it is if you throw a hissy fit and refuse to get the feature done if you don't get your way. You are paid to create value which is in delivering features and fixing bugs. Architecture is supposed to be there to help you.
Think about it for a second. If your architectural purity demands a hefty refactor, is that a good justification to postpone a bug fix or delivering a feature? No, it isn't.
> The problem is people don't want to expend the very minor extra amount of effort.
Except that most time it isn't minor.
You have to pass interfaces throughout a bunch of components, which require updating all sorts of tests, and result in a large code footprint.
> After a while you have a giant festering pile of shit (...)
That only happens if you're incompetent at your job and fail to do the most basic maintenance tasks in your code base.
Look at what you're trying to claim. It's supposedly easy and trouble-free to put together a PR that respects your personal notion of what the software architecture should be. Yet, the work to refactor code that breaks it is so insurmountable that you're no longer able to refactor it back to shape?
Which one is it then?
It looks like the FANG engineer you tried to badmouth had a firmer grasp on things and on what it matters the most than you do.
Re: Don’t teach during code reviews
#284Earlier 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.
Designing features, architecture and learning should happen before someone even gets to the point of submitting a PR. Plus if it can be improved on in follow PRs then do that instead of blocking juniors with pedantics
Re: Don’t teach during code reviews
#285Earlier 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…
Re: Don’t teach during code reviews
#286Re: Don’t teach during code reviews
#287Hm. 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 ( 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 leav…
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…
Re: Don’t teach during code reviews
#288Re: Don’t teach during code reviews
#289Earlier quoted context omitted.
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…
"Separation of concerns" as in splitting a simple thing into 12 files because you've had too many books about architecture?
I get it, you've been burned out by Enterprise(tm) code. Going to the very edge of the other side isn't any better