Things to look for when submitting and reviewing pull requests

My attempt at distilling the important components of self and peer code review so you can move fast and break fewer things.

Transcript

2. The purpose is to: • Minimize bugs • Minimize the potential for bugs • Make it as easy as possible to recover from bugs • Keep complexity under control so the code is kept flexible and easy to work with 2
3. Before submitting a pull request Before even committing a line of code 3
4. Make sure you didn’t catastrophically break the project 4
5. Sometimes you get errors and warnings that don’t break the project, but that doesn’t mean you should just ignore them. 5
7. Because something’s going to break eventually And when that happens, spotting what caused a bug is like finding Waldo. The less noise there is, the easier it will be to find 7
8. Similar reason When something breaks and it’s hard to pinpoint exactly where the offending piece of code is, one of the things you do is to start rolling back commits to find the one that broke the project. Once you find the commit, you would have drastically narrowed down where the culprit could be hiding 8
9. That’s not nearly as effective if your commits are huge. So get rid of anything that doesn’t need to be there 9
11. These are a lot harder to review 11
12. Imagine if the designer changes their mind and suddenly they don’t want a bouncing donkey animation anymore Would be nice to merge that bugfix in, but you can’t get one without the other. 12
13. If you branch off your previous feature branch, the one you just submitted a PR for, and you submit another PR for the feature you’re working on now. The 2nd PR would be more likely to contain code from the first 13
14. What are liars? They’re things that “are” or “do” something other than what their names suggest. They don't usually start off wrong from the outset, but as your project's code evolves, some functions might start straying from their names. 14
15. Imagine a carousel, that, when clicked on, advances to the next slide. But then requirements change. They want to disable that on mobile 15
16. You get a pull request and see a diff that looks something like this. What’s the problem? The problem is - the function isn’t “goNext” anymore, it’s “goNextIfNotMobile” Imagine if requirements further change, and swiping left is supposed to make the carousel go to the next item. First of all, the developer who implements that might not know that goNext isn’t actually goNext. And after he 16
17. figures that out, what do you do? Make a function called “actuallyGoNext”? Add a parameter to override it? 16
18. A function’s signature isn’t just the parameters that it takes, it’s also the type of value it returns. These should change as infrequently as possible. Whenever they change, you gotta look for if there’s anything anywhere else using that function and expecting its previous signature 17
19. What’s wrong with this? 18
20. Brief tangent I think we should use flow Compile time type checking gets rid of a lot of errors 19
22. This was part of a pull request I’d gotten about 2 weeks into the project I saw it, sat down next to the developer and we pair programmed for about half an hour and turned it into : 21
23. This. Sometimes it’s worth it, sometimes it isn’t You have to make a judgement call 22
24. Note that they’re all estimates 23
28. @cheapsteak Github.com/cheapsteak 27