You’ve been there. You’re sitting in a meeting and your boss, a product manager, or an executive is talking about Q2 goals. They’re laying out a roadmap of the features that are going to be “coming down the pike”. All of a sudden you see it. An innocuous bullet that makes your blood boil: “Auto-invite friends”, “Re-engagement notifications”, or “Disable ATS”.
The particular feature isn’t important. What matters is that you’re the engineer that’s noticed this capital-B Bad Idea. You know why it’s a problem. This time it’s not just the technical debt or the time it’d take to implement. This idea is bad because it trades a worse product for a better “business”: revenue, eyeballs, impressions, you know the drill.
You have a choice in this moment. You can stay quiet and hope it goes away or point it out, question it, and even argue against it. But so often, engineers fold. They ignore their conscience and their gut in the interest of a steady paycheck and an easier work day. Avoid conflict at all costs, especially when that cost could be their job. “Just keep your head down and do what you’re told”, they think, while they twiddle their thumbs as bad product decisions whoosh by. Sure, they complain about it over drinks with coworkers and in one-on-ones, but they don’t say anything when it counts.
We’re better than this. As software engineers and designers, we’re in the room when decisions are shaped, and the only ones who have the power to actually execute them. It’s our responsibility not to forsake the people who trust the apps we make with our silence. To stand up and refuse to implement unethical systems and dark patterns. And even more, to educate stakeholders on the real human costs of their business decisions: the time, attention, money, and trust of their customers.
It’s harder, yes, and riskier. But they can’t build it without us. We get a say. Even if it’s not in that meeting, we can think about the goals they’re trying to accomplish and propose alternatives. We don’t have to hide in our sit-stand nap pods and eye-roll while we engineer a worse world. We can do more than write code. We can research and present better alternatives. We can write memos and make a slide decks to convince them of of our position. We can be activist engineers.
Even though these bad ideas may buttress the metric-of-the-week, they’re at the direct expense of consumer trust and customer satisfaction. They’re a tax on our company’s reputation. We have to push the people making the decisions to measure more than just the number they’re trying to increase. Look at reviews, net promoter score, social media mentions, and team morale. All of these trends matter to the long-term health of the company, and should be treated as such.
This requires long-term thinking and the kind of organization that’s receptive to it. In many companies, quantifiable short term gains are valued more than long-term, qualitative investment. The best companies resist this temptation to make a quick buck and build upon a lasting mission and principles. But even in companies with lofty vision statements, things can go awry. A bad quarter can send the company’s hard-won principles out the window to make room for the growth hackers.
In other disciplines, engineers wear an iron ring to remind them of their commitment to their profession. Though we may not be part of the Order of the Engineer, we can learn a lot from their obligation:
As an engineer, I shall participate in none but honest enterprises. When needed, my skill and knowledge shall be given without reservation for the public good. In the performance of duty, and in fidelity to my profession, I shall give the utmost.
Of course, not every idea you dislike is a bad one, so spend your reputation thoughtfully but forcefully. Make your dissent count, but don’t be a jerk.
Our job as software engineers is to build things that make the world (or a corner of it) better, things that solve problems. But that’s not our only job. It’s also to be gatekeepers: to prevent ideas that we know are harmful from being realized. What’s the worst that could happen: we get a reputation for giving a damn?
You’ve been there. A 10,000 line pull request lands in your email and you don’t even know where to begin. No description provided.
Should you start by installing it, running the test suite, or should you just start scrolling though while your eyes glaze over from the red and green stripes? Is this developer really looking for feedback or are they on a deadline and pressuring you to say “lgtm”. Will your one innocuous comment ignite a flame war?
Code review, or more generally peer review, has a long record of finding defects in not just software engineering, but in science, academia, and many other industries. While some argue about the specific percentages of issues that it finds compared to an automated test suite, or others method of testing, code review is an incredibly useful tool in any software teams arsenal against bugs and poor software architecture. But only if it remains a sharp one. Laziness can creep into code review very easily if you’re not careful.
Jason Brennan has written two great pieces on the topic of diligently writing and reviewing pull requests, and I’ve used his series as an inspiration for my own system. I’ve also found it’s handy to have a short-form checklist to go over when performing code reviews to remind myself what I’m going for. Maybe it’ll help you and your team too.
For the engineer drafting the pull request
Provide a screenshot, GIF, or video of the change if possible. People like pictures.
Explain what changed and why.
List step-by-step how to test the changes.
Link to any relevant task(s) or ticket(s) in the bug tracker.
Link to any existing documentation that could make the change easier to understand for the reviewer.
Mark any areas that are work in progress or require follow up.
Note anything that is waiting on other departments or team members.
Call out any legal, security, or privacy concerns.
If any third-party dependencies have been added, explain what they are and why you chose them.
Double check that the code is styled, documented, and tested to your team’s standards.
Mention people who would be interested in the changeset:
Engineer(s) who wrote the old version
Product manager (if they’re interested)
Give the diff one final pass yourself before asking others to take a look. You might catch a few silly things like typos in comments.
Annotate particularly tricky sections in the diff to make what’s going on even clearer. Maybe even turn these into code comments.
Get a coffee and wait for the constructive criticism to roll in.
For the reviewer
I find it’s helpful to do these in order if you can.
Most importantly, remember that the engineer on the other side of the screen is a person. Try not to be curt or hurtful in your comments.
The Pull Request
Start here to build an understanding of what you’re about to be diving into.
Let your teammate know that you’re reviewing their work however’s customary: a comment, a message, making yourself the assignee.
Read over the pull request description and any linked documents to understand the nature of the changes.
Ask questions about anything you don’t understand or if there isn’t enough detail to properly review.
Does it work as you’d expect? You can write lots of pretty code that doesn’t work.
Alright, it’s time to start looking at the code, line by line.
If code was deleted, was its functionality adequately replaced or is what it provided no longer required?
Does the new code introduced conform to the teams’s standards for style, documentation, and testing?
Does new code make sense when read? Is anything too clever or inexplicable?
Is new code safe?
Does it have the potential to crash or hang?
Are there any obvious race conditions or concurrency concerns?
Is new code fast?
If not, can you suggest optimizations?
Is it well-designed and well-factored?
If not, try to propose alternative solutions or schedule a time to whiteboard with the engineer.
Is it well-named?
Are the names of types and functions obvious and unambiguous?
Do you understand why the modifications were made?
Do the modifications improve the factoring, design, or performance of the software?
Do the modifications change any of the fundamental assumptions that the original code made?
Make sure to understand the implications of new dependencies on third-party code, including their licenses.
Assets and Resources
Do all assets and resources exist in all the right formats and sizes?
Are they finalized and ready to go into production?
Do you understand the effects of these changes and how they can be rolled back?
Adding a system like this to my own pull request reviews has helped me catch a lot more, but I’m sure there are still things I’m missing. If you have ideas about how this checklist can be improved, pull requests are welcome!
A few years ago I was tasked with helping to recruit interns for The New York Times iOS team1. I traveled around to top-tier engineering programs at universities all over the northeast talking about the Times’s engineering culture and to students about what they wanted to be when they grew up.
One of these interviews stuck with me. I asked a junior what he saw himself doing in five years, and I’ll never forget what he said.
“I want to be the idea guy at a startup”.
I wish I could have stopped the interview right then and there. I wanted to tell this guy that there is no Santa Claus, that the Easter Bunny isn’t the one hiding the eggs, and that no such role exists or ever will.
Some nights I wonder where that guy is. I’m scared to look through my notes and check up on him, but I sincerely hope that he’s found some way to bring his ideas into the world — something to do or to make.
I wish we hadn’t separated those two concepts in English. ‘Faciō’, one of my favorite Latin verbs, encapsulates both concepts in a beautiful way. Anyway, back to doing. The ideas will come.
By the way, they’re still hiring. If you want to work there, or at Tumblr for that matter, email me. ↩