Hacker News new | ask | show | jobs
by EB66 645 days ago
This is a really interesting phenomenon that I've experienced before myself but I hadn't fully understood or appreciated as clearly as the author has.

At my company we put a big emphasis on code reviews. We encourage devs to pull request code fairly regularly to keep PRs relatively small (when possible) -- before so much code has been written that it's not really possible to change course without blowing up deadlines. We encourage our junior devs (who might not be capable of identifying bugs or proposing fixes on code written by a senior) to ask questions in their code reviews -- to verify assumptions, to request an explanation of how something works, confirm that a particular edge case has already been considered, etc. It can be hard to get a junior dev comfortable with doing this (questioning a senior dev), but even if the junior isn't identifying bugs it will often lead the senior to better understand their own code and the architectural concepts that underpin their own coding decisions. Like the author points out, this only happens because the senior dev endeavors to explain their work to the junior dev (Protege effect). Also, a good many times it leads the senior dev to re-consider how they wrote something and they might add a revision to address a possible edge case not previously considered. I hadn't thought of it this way before, but this is the Socratic method that the author talks about.

We also put a big emphasis on in-code comment writing -- largely following the commenting principles laid out by John Ousterhout's "A Philosophy of Software Design". These comments are of course for long-term maintenance purposes, but they also benefit team learning. Class, method and variable naming are obviously important too. Our internal code reviewing mantra is that 'I want to be able to read your code like a story book -- when I get to the end, I at least want to be able to understand what happened'. Not always possible, but a good goal. Writing comments and choosing class/method/variable names in pursuit of that goal massively contributes to the learning of the team. During our code reviews, one of the most common requests by reviewers is for the author to add a comment explaining something that was very difficult (or impossible) for the reviewer to grasp on a first read.

This approach has worked very well for us. Everyone learns and our product quality improves.

5 comments

You have to be OK getting rid of productive assholes that mess up this culture. The hint of a senior not being open to criticism will instantly halt any ability for a team to have a safe, open dialog.
i’ve worked with people like this - big egos were like a bag of lead the entire team had to carry.
Equally true when they're good and bad, unfortunately.

In the 18 years since I graduated (I feel old), I have seen some people easily carry entire projects while demotivating everyone around them… and also colossal idiots who were just as confident and unwilling to listen.

I really hope I never end up unable to listen to critics.

We've found good result hiring candidates who've experienced some form of ego-death. It's a difficult signal to separate from the noise of the talent pipeline, but until people start filling their resumes with meditation buzzwords, it will suffice.
How do you select for this? I am significantly less cocky than in my youth, simply because I have accumulated a long list of personal failures (ie teaching moments). Failings which I would not want to discuss with a potential employer.
I wonder if this why Steve Jobs preferred to hire those who had experienced LSD. I think I saw a video of him mentioning this.
Yeah, very true, when we're hiring we'll sometimes skip over the most talented candidate and pick a candidate who we feel would be the better communicator, easier to work with and fit in with the team. Easier said than done, but we at least try to hire for that.
This thing with small PRs bothers me somehow. Where does design happen? Do we just make endless tactical changes?

For juniors this is even worse, they get to wait until PR time to validate their design. With trunk based development this means continually commiting unreachable code with back and forth with more senior staff via PRs, instead of sitting down for an in depth discussion where they'd actually learn how to do this job.

This isn't really aimed at the parent poster, just an observation. I've been working since 2006, GitHub brought in PRs and I think we lost something over night: code reviews where you get to sit with more senior staff and discuss your code.

I've been working through PRs since around 2011 and I don't think I've ever seen a place where PRs were intentionally used to handle design issues. Occasionally design problems will get brought up in PR, but that's always been considered a failure of earlier planning when it happens.

Depending on the company, the design discussion you're talking about has happened:

1. In verbal discussions with other devs before fingers ever touch keyboards

2. In long-form writing (Text docs, then Google docs, then later Notion)

3. In per-project slack channels

4. Out loud, on a whiteboard.

Right now it's very much #4 because my company is 2 devs and a founder. But even here, I wouldn't expect to get anything non-trivial into pull request before figuring out the overall structure with the other dev verbally or in writing.

For non trivial work do you expect it to be delivered via small incremental changes? Genuine question. Not trying to but pick or win an argument.
When possible, yeah. I imagine some sort of golden rule: "make PRs for others to review that you would like to review yourself." There's plenty of room for exceptions there - my coworker is currently reviewing a 1300-line PR after my apologies for it :)

But yes, we generally break things down into the smallest meaningful chunk possible to deliver. If that chunk is too small to understand the larger work that it's part of (as is usually the case for non-trivial work), we can discuss the larger work in another forum (ad-hoc verbally, via design doc, via meeting, etc).

At my first job a few years back I sat down with a senior dev at least once a week (informal meeting) to discuss my tasks and the code bases we were responsible for in general. It was incredibly helpful. I could also request feedback at any time. At the end of it, the final PR might be large, but it had mostly been discussed and reviewed by then, so doing a final review was more of a formality, dotting the i's and crossing the t's to ensure the criteria had been properly fulfilled.

I thought it was a great process.

I think I'm going to suggest that if I'm assigned as a mentor for junior staff again. Thanks for sharing it. :)
In addition, writing code that is accessible to juniors (which should apply to a large majority of a code base) increases the velocity of everyone on the team. Readability, modularity, and consistency create standards and examples for a junior to work by, which is a great way to learn and lower the risk of bugs and surprises.
Yup. This is why I favour simple architectures like MVC over complex ones like VIPER.

While there may be exceptions, I default to: if an app is so messy as to consider a more complex architecture, it probably has so much going on as to be difficult for the end user to use it.

So that : Always Optimize for Junior Devs.

https://blog.pwkf.org/2022/09/18/always-optimize-for-dummies...

> ...code reviews

> ...PRs relatively small (when possible)

> ...deadlines

Truth is if you are doing the kind of work that fits into code reviews, small PRs and neat deadlines, does any of it matter? You're describing a tech enabled agency, not a startup. Sure customers have alternatives, but in the same sense that we have choices when booking airline tickets: you are describing a development process for a product that is fundamentally fungible, so the main differentiator among producers is costs, not management strategy.

Why on earth would a startup's code not be reviewable?

If I'm doing a big chunk of work, I still do it in small PRs. That first PR (or a design doc) might outline a strategy for the ones that follow, but still try to have small PRs. Or, worst case, I do a big chunk of work and break it out into small logical chunks for review.

And even startups can have deadlines.