Hacker News Reader

Comment by jongjong | Hacker News Reader

My problem in my last role when I read large Pull Requests is that they tended to be way more complicated than they should have been but because they worked and I couldn't single out a small number of specific problems, I had no choice but to approve. Still, I knew it would slow us down in the medium and long term but this bloat is completely invisible to management.

It has become taboo to say things like "This code is too tightly coupled", "You don't need to add an external dependency for that", "The inputs to those methods are too complicated; your components are micromanaging each others' state", "You're violating separation of concerns", "The cyclomatic complexity of this code is too high; you could simplify all your if-else statements"... When it's not my company, it's impossible to dismiss code when it works right now, even though it is likely to break later.

View on HN

I'm honestly not familiar with this "I had no choice but to approve" mindset. In the greenfield projects I've worked on there was always at least one person, sometimes several, who had the authority to just say "no, this is far too complex, scratch it all and we'll meet tomorrow to discuss next steps." Sometimes it ended up with breaking the PR into several more manageable pieces, sometimes it ended up with a wholesale rewrite / refactor of that component.

I didn't have enough leverage in that company to say such things and the project had already been running for a couple of years when I joined (not greenfield).

The last greenfield project which I managed from scratch, we didn't have this problem because all developers shared the same mental model of what we were building before we wrote any code. We had a lot of discussions beforehand to get to this shared understanding. There was literally not a single PR which surprised me throughout the entire project and I'm sure none of my PRs were a surprise to any of my team mates either. There was plenty of disagreement throughout but it was always fully resolved through discussion before we started coding each major feature.

In my experience it pops up when company politics get involved, or somebody gets attached to their idea about how certain things should or shouldn't be done. In that case often the safest thing you can do for your own reputation and appearance is to leave some very gentle notes that a certain thing maybe could have been done differently, but approve it anyway on the grounds that it works for now. That way you're not seen as an obstructionist, but if things do go wrong you at least have a written record so you can say "I told you so".

I sometimes use my job title as a higher level senior or lead to say vulnerable things like:

"This change is hard for me to read and understand"

"This is a large PR, and it may be difficult for me to schedule time to review it. Can it be split up into a series?"

I also configure linters and code quality tools to automatically flag some of the more egregious problems.

I can relate though I wish I could get into a position that I could say such things and not get fired. In some circles "This change is hard to me to read and understand" would be interpreted as "I'm an aging dinosaur who doesn't understand this new tech; you youngsters are too clever for me." (though I realize how completely wrong it is).

I'm 33 but I feel like I already have to make an effort to avoid the dinosaur label. I disagree with a lot of modern tech trends but I simply cannot express my view about them even though I could explain the problems very clearly and logically and can provide far better and simpler alternatives. Unfortunately, hype does not yield to reasoning... And sometimes, you're too far into the tech debt and it doesn't make financial sense to rewrite.

I’m 10 years older, my experience has been that getting to ask stupid questions is one of the joys of age/seniority/security. Very often everyone else in the room has the same stupid question but you get to look like a stone cold genius because you were willing to risk looking silly.

I guess maybe in 10 years I'll be working with 30 year olds who understand and value of that approach as I do today.

My current reality is that I'm a 33 year old working with 20 year olds who think they're geniuses who are going to take over the world in 5 years; from that viewpoint, I'm essentially a failed engineer because I didn't build a Facebook, Uber or AirBnB even though I had 10 years to do it.

From seeing your posts, I think you have a psychologica lock that under mines your self-confidence.

I'm curious what type of company has these team demographics. Startup, agency, SMB, bigcorp, academia, or?

Usually this kind of thing comes from “trying to keep costs down” at a poorly funded firm, usually startup-ish. I’ve worked places where the oldest engineer was 30 and it’s rough, quality and stability just aren’t in the average 25 year old’s toolkit.

Don't mislead yourself, that benefit comes exclusively from security. Age and seniority only contribution is some weak correlation to security.

My secret weapon (rarely needed) is “I got this eventually, but someone may have trouble quickly figuring it out during a 3 AM outage.”

> "This change is hard for me to read and understand"

Believe it or not, I consulted at one place where the manager decided I was the problem because I was consistently assessing problem code and team processes in similar ways. She asserted that "everyone else understood", even when they plainly didn't understand but where just going through the motions.

Said manager had a number of other issues as well. Worst gig I can recall having in the last couple of decades.

I would start being less diplomatic once I get the hunch polite phrasing isn’t getting through - “this code is badly structured, brittle and will make debugging harder”. As programmers being tactless is expected so might as well use it to your benefit.

In this case, I could have, but I felt my time was better spent thinking about why things were complex and hard to understand and made notes on that, mostly for my future self. Whenever I found the opportunity, I would include some of the reasoning and explanation, in written form, in my feedback. I didn't get much out of that gig, but I did end up with several thousand words of ideas and observations. I wouldn't be surprised if some future programmer on that project ran across something I left behind and found it useful, or at least comforting.

Edit to add: in 1:1s with the manager I was more direct. About the best I can say about that is "at least I tried".

"You don't need to add an external dependency for that" "You're violating separation of concerns"

I've found that kind of feedback to be useful actually. And when giving similar feedback it is received better with a small change in wording to make it more about the code and not the person. Even if that is the intention the wording does matter. For example:

"An external dependency isn't needed here, try implementing with XYZ instead." "Split this function into x and y for better separation of concerns"

Removing the "you" removes some resistance to receiving the message .

The imperative can rankle too. I like passive voice or "let's" statements.

Yes always “we,” never “you.”

I’ve become fond of mostly asking questions.

I wonder if we could avoid this dependency? Maybe [idea] would work?

Can you see a way to separate the X concern into a module?

Can we split this function up a bit so the nesting won’t be so deep? Would this loop body make a good standalone function?

Love this, it's the Socratic method and leads to a better path without telling someone "how" to do their job.

Somewhat unrelated, but it's true that it's sometimes hard to give a reason to reject code that has a "funny smell".

Without going into specifics, there was a case where I reviewed a PR, asking "this isn't usually how people do file operations, are you sure this is really fine?" To address my concerns, they even wrote a specific test program to "prove" that there weren't any problems with the code. I reluctantly approved the PR since I couldn't just ask them to rewrite the whole patch due to just a hunch.

Lo and behold, after a kernel upgrade and file system change, that weird piece of code caused a multi-week panic for the whole team. The extra funny thing is that the test program above made it trivial to confirm and reproduce the issue once we identified this was the cause.

Right. This is where you would think the AI assistance could play an important role:

Warning: it looks like you’re trying to pack too much crap into a single PR and your peers are unlikely to understand what they are accepting.

The person who would ignore the reviewer’s recommendation in this case, would probably also ignore the AI’s as well.

Reply on news.ycombinator.com