Code doesn’t have to be a mess

This. We need to stop using time pressure as an excuse to do a bad job. Moving things out to a separate function might add 10 min but save 100x when everything comes crashing down.

Of course you shouldn’t overengineer but so much can be gained from spending a little time just thinking about how this should work.

We’re amidst a rewrite and we have an off-shore team involved. They same team that built the original starting 4yrs ago.

One of their team members decides “we should validate the TLD for email addresses entered by the user.” Code is added, a TLD file is added, and in code review I reject the whole concept. Show me the ticket or feature docs, and I’ll argue with the author of those instead.

“We did this in the last app…” Maybe, but we didn’t spec that for this app.

He got his local project lead (non-tech) to write a Jira story for us to “discuss the technical implementation.” Dude, srsly.

Our app is web-first and is used on congested mobile networks (like, hundreds of people all using the same cellular site simultaneously.) A TLD file does not need to be delivered to each of them for validation that’s pointless.

The idea is off the table, code rejected, but the guy spent time doing something no one asked for and had his local team onboard with it.

  public class EndpointManager {
    private EndpointInputManager eim;
    private StringSplitter splitter;
    private NumberParser parser;
    private Sorter sorter;
    public EndpointManager(EndpointInput input) {
      eim = new EndpointInputManagerFactory().setInput(input).build();
      splitter = new StringSplitterFactory().setDelimiter(new Delimiter(",")).build();
      parser = new NumberParserFactory().setFormat(NumberParserFormat.INTEGER).setMode(NumberParserMode.LIST).build();
      sorter = new SorterFactory().setSortOrder(SortOrder.ASCENDING).setAlgorithm(SortingAlgorithm.QUICK_SORT).build();
    }
    public EndpointOutput endpoint() throws ParseException {
      splitter.split(eim.getInput());
      parser.parse(splitter.getList());
      sorter.sort(parser.getOutput());
      return new EndpointOutputFactory().setOutput(sorter.getSortedList()).build();
    }
  }

- Someone says "build this thing"

- I build "this thing"

- That someone says "just kidding, we're not gonna use it"

- I'm peeved

Someone else in this thread is arguing this is an entitled position, and here you're arguing that... well, I think you're arguing that I think all my code is always amazing and should always be merged.

I'm not! Like I wrote elsewhere I've written some pretty shit code, I've built the wrong thing, and I've built broken things. I'm sure this is true for most SWEs. This isn't the scenario I'm describing.

But I think this discussion has some merit in terms of how we navigate code review. For example, conversely, I've been on the other end of some pretty... bad feedback. The first example that comes to mind is that we had a portal where you could search by text or category, but once you selected a result we wouldn't save your search anywhere (query params, session storage, etc.). Consequently, when you clicked our "back" link, your search would be gone. We YAGNI'd it for a long time, but we accepted a very tight deadline project (COVID/government related) that required a category that needed to be sticky.

I built this using query params, like pretty much every search out there (for good reason). This ended up changing a lot of templates, a couple of front-end React components, and required extra logic in a couple Django controllers. It was a big-ish change, maybe (to my recollection) 300-400 lines across a few stacked PRs--meticulously, for ease of review. All previous tests passed, all the new tests I wrote (typically >= 50% of my PRs were new tests) passed, I even built a punch list of UI tests I ran through (this was going to be a big user-facing feature and I wanted it to be bulletproof). This took I think... 2 days of constant work, so something like ~30 hours.

This wasn't our typical process; we skipped our usual engineering meetings about implementation strategy and what-not. Our team was small--4 people including our CTO--but even so we had a wide diversity of opinion when it came to implementation, architecture, and style, so it kind of ended up being the case that if we wanted anything to get through PR we had to hash it out beforehand. But we literally had 7 days or something to do this, so we just didn't have time.

But, predictably, despite all my tests and punch list, my PRs were rejected as "too much code", and we missed our deadline. We launched without the feature. Our CTO reviewed the vast majority of our PRs, he reviewed these and he was pretty furious about the scope of the changes, blaming me for missing the deadline.

Afterwards, he tried reimplementing it using query params in less code, but failed. He then tried reimplementing it using local storage, which was less code, but had multiple problems: local storage works across tabs which is deeply weird, but even if he switched to session storage, it didn't work in lots of versions of mobile Safari if you're in private browsing mode. I rejected that PR for those reasons, which we disagreed vehemently about. Eventually, a couple months later, we paired on it, and basically reimplemented my work together.

There are obviously a lot of flags in this little story, but I don't think they're wildly out of the ordinary for a startup (if anything, it's way too much process for a 10 person company). My point is that, while I'm sure there are a lot of cases of "I'm God's gift to this company merge all my work never question me" out there, there are also a lot of cases of "no PR is fit to merge the first time" and "I'm a great programmer, you didn't do this the way I would, therefore this isn't good enough" as well.