There was a meme I saw recently which described so-called “Shopping Cart Theory”. It's an amusing spin on the moral dilemma from philosophy known as the The Trolley Problem, which is also its own [amusing meme](https://knowyourmeme.com/memes/the-trolley-problem) (because in the Internet Age everything has to become a meme). The obvious question the memes poses is basically: are you a moral person?

![](https://cdn.upworthy.com/440106/uploads/bf52fba0-99ad-11ea-8867-43e46fa5810e_800_420.png)

I think the actual question of the meme can basically be boiled down to: “do you do something for the greater good even when you don’t benefit from it?” It’s an interesting question if you shift the context to software development, specifically Pull Requests.

## The problem

Assuming you’ve worked professionally as a software developer for at least a year or so, you’ve almost certainly been asked to review some monstrosity of a Pull Request (PR) where the developer who opened the PR didn’t make the slightest attempt to reduce the work you would have to put in to understanding the PR.

It might have been a PR with several hundred, or even over a thousand lines of code changed. Maybe it was a PR that changed a sizable amount of configuration in a way that didn’t make sense or lacked context. Or maybe it was simply a PR meant to change the appearance of some UI element but the PR didn’t actually have any before or after screenshots.

In all of the above cases, as well as plenty of others that I didn’t bother to list, the issue is that the engineer who created the PR is putting a burden onto the engineer reviewing the PR. 

You can probably see why I compared this to the Shopping Cart Dilemma. 

In the Shopping Cart Dilemma, you’re given the choice of returning the shopping cart to the designated area and reducing the workload of the employees at the store. It also reduces issues caused by shopping carts being scattered around parking lots (things like shopping carts blocking parking spots or shopping carts getting dinging vehicles as the carts get blown around on windy days). 

Alternatively, you can take the selfish route and simply leave the shopping cart abandoned in the parking lot, which will require more work from the store employees to gather and/or lead to inconveniences in the parking lot for other shoppers.

With both the Shopping Cart Dilemma and terrible PRs, the root issue is the same: **individuals choosing a selfish path that increases their own convenience by inconveniencing others. **

I’m going to assume you’re an adult and I don’t need to tell you why being “that person” makes you a garbage human being, so instead I want to focus on the steps you can take to avoid being “that person” and instead create great PRs that make life easier for your teammates.

## Providing Context.

If you think about the hardest Pull Requests you’ve had to deal with, the issue is always that it's hard to ascertain all the required context surrounding the code changes. If you can’t understand the context of the changes, it’s difficult to confidently approve a PR. 

Sure, a dev doesn’t need to know the context of the changes in order to catch things like typos, obvious bad practices, or certain logical bugs in the code. But that's a small list of things compared to everything that a reviewer could catch in a PR where they actually have context.

When a reviewer has all the context necessary to understand the changes being made as part of a PR, they can ask about edge-cases the PR author might not have considered; they can discuss other approaches that might better solve the problem(s) the PR is meant to solve; they can provide additional context around the problem that that the PR author themselves might have been unaware of. 

There's a long list of good things that can come up when a reviewer has all the context necessary to understand a PR. But if you create a PR that prevents reviewers from understanding all the context, then you deprive them of the ability to provide a thorough review while also depriving yourself of what might have been useful feedback on your PR.

Obviously, that’s not good. 

Instead, you should seek to maximize the amount of context you provide reviewers, at least up to the point where they know everything they need to know in order to review your changes. This improves your odds of receiving thorough peer reviews and merging in the highest quality code you can after addressing any feedback.

## So, how do you actually achieve that?

Well, I’m currently a frontend engineer so the steps I'm going to mention may not cover everything worth doing if you’re a backend engineer or a data scientist. Nonetheless, something I think is universally applicable is...

### Reviewing your own PR after you open it.

As soon as you open your PR, start going through the git diff and adding comments. Great comments generally explain why something was done. These are changes that might not be so complex they merit a comment in the source code but a reviewer might still have questions about why those changes were made. Anticipate those questions and try to answer them in the PR comments ahead of time so that you and your reviewer don’t need to comment back-and-forth on the PR to answer a question you could have easily foreseen.

Another huge benefit to reviewing your own PR and commenting as you go through it is that you may catch things that you didn’t notice locally. 

This happens to me all the time, where I’m reviewing my own branch after opening the PR and I realize I left some comments that I meant to remove or any of the other small things that are easy to forget about during local development. 

The diff that you see in your Pull Request is the exact same diff your reviewers are going to see, so if you’re seeing things during self-review that you know need to be addressed then it makes sense to go and fix them rather than waiting for reviewers to bring up questions about those parts of the code. 

Basically, if we can catch issues to address during self-review or anticipate questions from reviewers and address them by commenting on the changed lines to add context, then you reduce the workload of your reviewers and improve their ability to focus on things that you yourself did not notice.

This leads to the next big thing, which hopefully you were already aware of.

### Keep your PRs small.

Every programmer I know has had the experience of being asked to review a huge PR. We all hate it for the same reason: it’s simply not feasible to provide a good code review on a huge PR, at least not without dedicating a massive chunk of our time.

There’s a funny saying I heard that went something like “Give a programmer a 30 line PR to review and he’ll catch 3 bugs. Give a programmer a 3000 line PR and he won’t find a single bug.” It’s funny, but I also find it to be incredibly true.

There are many reasons that large PRs are less likely to receive a thorough review. 

The most obvious one is time. Asking someone else to thoroughly review 2000 lines of code that they themselves did not write is basically asking them to give you an hour or more of their time. At most companies, software engineers are all busy with their own work so when you ask other engineers to stop what they’re doing in order to dedicate a bunch of their time towards moving your own work forward you’re being a bit of an ass. It may not seem like it to you, but in these situations you have to realize that you are draining their time in order to maximize your own time, like some sort of Time Vampire.

Another issue with large PRs is that it's tough to understand all the context behind the changes and it’s also mentally fatiguing trying to catch every little bug. 

When you make changes that span hundreds or thousands of lines, there’s a decent chance you made that change over several days. To you, the change probably makes perfect sense. But the reality is that reading and writing code is a slow process even for experienced programmers, and so if you wrote 1000+ lines in 5 days, asking your reviewer to understand and review those 1000+ lines within an acceptable timeframe for peer review (~15 minutes or less) simply isn’t feasible.

As a result, the reviewers can try to catch as much as they can but they will almost certainly only catch a fraction of the issues rather than all of the issues they might have noticed if the PR had been broken into chunks.

The solution here is pretty simple: if you need to make a large number of changes, make multiple PRs rather than batching the changes in one huge PR. 

You can do this by either pushing into the main branch over a series of smaller PRs, or by creating a separate branch and then opening smaller PRs into that one before merging it into the main branch once all the smaller changes are approved. Both options are good and which one is “better” just depends on the changes involved.

### Providing non-code context.

Many Pull Requests include changes that aren't easy to reason about just by looking at the code but can be easily clarified by adding a little extra information. As a frontend engineer, the example that most obviously leaps to my mind is screenshots. 

If someone is making a change to the User Interface, I want to see screenshots in the PR  that provide a clear before/after comparison. I can get a decent idea of the changes made to the UI based on reading the code, but seeing the updated UI makes things undeniably clear. 

This is especially helpful if there is a design the PR author was actually meant to code against, such as a Figma spec from the design team attached to the original Jira ticket. If I’m your reviewer, then I definitely want to make sure that the UI you’ve created actually lines up with the UI design proposed, but if you don’t provide me with screenshots that make it easy for me to compare the before/after then my only other option is to clone down your branch and run it locally to see the changes. Again, asking your reviewers to do this because you were too lazy to grab the screenshots yourself is disrespectful of their time.

I’m not totally sure if there are any backend or AI equivalents to the example I’m giving here, but the underlying idea is the same: if there is additional context you can give to your interviewers which might not be clear from the code changes alone, make sure to do so.

## A Quality Codebase requires Quality Code Review

While some individuals may be “better” at coding certain things than others, no one is a master of everything. More importantly, we are all human and even the best engineers in a company make mistakes. With that in mind, the only way to truly build up a codebase that is full of quality, thought-out code is through quality peer reviews.

It’s easy to get lazy if you’ve been coding something for a long while or you’re under pressure to deliver as quickly as possible, but making low-quality PRs doesn’t help your team overall -- it only helps you. 

So to return to “The Shopping Cart dilemma” I mentioned at the beginning of this post, if there is no dire emergency will you take the time to make higher quality PRs even if the benefits aren’t obvious?


Creating Great Pull Requests

Nobody knows everything. 

I know that. You know that. Pretty much everyone knows that.

But despite understanding that you can’t be expected to know everything, new developers often have an aversion to admitting that.

This winds up hurting their employer, their team, and themselves.

If you’re looking to improve your engineering abilities as quickly as you can, then you’re better off exposing your ignorance, a mindset that I learned from _Apprenticeship Patterns: Guidance for Aspiring Software Craftsmen_. 

## Understanding The Problem

Let’s pretend it’s your first week in a new software engineering role, and you really want to make a great impression.

Obviously, the best way to make a great impression is to do great work, right?

So you start working on your first task and bump into some strange behavior. Perhaps you’re trying to fire a fetch request from the frontend to some internal API, but the request is failing inexplicably.

Or perhaps the request is returning unexpected data that differs in development from production. Or maybe you’re not even working on a fetch request but are actually trying to start a Docker container locally and it’s bugging out.

**Regardless of the roadblock you stumble into, you need to be quick to expose that roadblock**.

Your employer doesn’t pay you to be stuck. You get paid to deliver features on time and get unstuck quickly (and hopefully do other things as well like [Glue Work](https://noidea.dog/glue).

## The Truth Will Set You Free

When you get stuck or are unsure how to navigate a certain task, admitting that to relevant people (teammates or your manager) is the fastest way to solve the problem.

Assuming you’re not working in some sort of nightmarishly toxic environment, your teammates and manager are going to be okay with you asking questions, especially during your first few months on the job.

Frankly, if I had a new hire on the team and they _weren’t_ asking a bunch of questions I’d assume they’re making the exact mistake I’m talking about here -- being too afraid to expose their ignorance because they fear they’ll be judged negatively.

## It's Alright to Get Stuck. It's Not Alright to Stay Stuck

One thing to make clear: I’m not suggesting that you immediately start pinging your teammates/coworkers as soon as you have a question about something.

Part of being an engineer is having a mindset that attempts to solve problems without immediately asking other people to solve them for you. 

Sure, there are plenty of exceptions to this advice, but in my experience, it's rare that software engineers are bugging teammates for help too often. It's more common for them to be reluctant to bug teammates even after they’ve been stuck on a problem for a while.

So the clearest guideline is this: you should exhaust most of your usual resources before you start directly messaging anyone for help, but you also should be mindful of when you’ve run out of low-hanging fruit to search.

This could mean checking your company’s internal documentation, such as Confluence, to see if your question has already been asked before, or using the search bar in Slack to see if the issue was mentioned previously in any work channels. 

That’s assuming that you’re stuck dealing with a problem that you think is specific to an internal process, such as connecting to your company’s dev environment. 

If you’re stuck on something code-related, such as trying to figure out how to make your frontend styles match the design included with a work ticket, then you’ll want to spend much more time trying to solve the issue on your own. 

That means busting out some Google-Fu to search for anything that might get you unstuck, including StackOverflow posts, Medium articles, language/framework documentation, etc.

Basically, if you’re stuck trying to figure out an internal process, you’re going to be limited to a quick search of internal documentation before you turn to a teammate or manager for help if your own search doesn't turn up any answers. No amount of Googling will provide clear answers for the internal processes at your workplace.

## Documentation as a Superpower

One thing that I think all developers should do, not just new ones, is balance exposing their ignorance with creating documentation.

If you had a question about something internal and weren’t able to answer it using your work’s internal documentation, then that indicates an obvious gap you should fill.

After all, if the thing you were stuck on had already been documented, then you wouldn’t have been stuck in the first place. And if you had a question about something, there’s a 99.9% chance some future developer at your work will have the same question.

I bumped into this exact situation at work not too long ago.

I was asked to deploy some recent code updates to production, but I had never deployed to production within our system. Deploying changes to our dev and staging environments was totally automated, but production deploys were manual.

So I dug around our own internal documentation and Slack briefly, didn’t really find any answers within ~10 minutes, and then reached out for help.

It turns out that our deployment process was basically tribal knowledge -- it was being passed from developer to developer but for some reason hadn’t been fully fleshed out as a guide or checklist in any internal documentation.

So rather than allowing that trend to continue, I wrote up what I learned from my teammates along with my own thoughts after executing the process myself so that future newbies to the team don’t get stuck.

That doc now serves as part of our onboarding process for new hires, and I looked pretty good for writing it.

None of my peers or higher-ups “looked down on me” for not knowing an internal release process. Instead, they were grateful that someone finally wrote a pretty crucial piece of documentation.

I mention this story just to give you some ideas if you are newer to software development and you’re still apprehensive about looking like a newb when you start that next job. The easiest way to build your confidence in asking questions is to balance that behavior with documenting the answers to those questions.

## Wrap Up

There is nothing necessarily new or mind-blowing in this blog post. 

The reason I wanted to remind you that it's okay and even a good thing to expose your ignorance when you are stuck/unsure is because it is usually a win/win situation.

One of the many distinguishing features between more experienced engineers and the more junior ones is that the experienced ones have a much stronger sense of when to start reaching out for help, which means they don’t stay stuck longer than necessary. 

Of course, they’re also not reaching out for help on small issues so frequently that it impacts the productivity of their teammates. There is somewhat of a balancing act at play here, but I’d recommend you err on the side of asking more questions rather than asking too few. 

You’ll learn where the line is as you go, and in the end, you won’t feel quite so ignorant.



Expose Your Ignorance

I was talking with some friends about React performance and eventually, a question came up that led me down a rabbit hole:

_Why not wrap all of your components in React.memo()?_

If you're not familiar with `React.memo`, it's a higher order component (HOC) that wraps your custom component and attempts to reduce rerendering by checking the props being passed to the component.

If a rerender is triggered by a parent component, the memo HOC will check whether the props being passed into the wrapped component have changed or not. If it detects that the props have not changed, then it does not bother rerendering the wrapped component and instead returns the last rendered result.

The nice thing is that it only checks props being passed into it from a parent component -- it doesn't prevent rerenders caused by changes in internal state or context values.

So this sounds like an easy win, right?

If the props we're receiving are the same, then why would we want to rerender? And if the props we receive are different, then we still rerender properly, so what's not to love?

And this is something me and my friends wondered for a bit. This behavior sounds awesome, so why not make it the default behavior of React?

Well...

## React.memo fails under a lot of unexpected conditions

According to the React docs, React.memo's default behavior "... will only shallowly compare complex objects in the props object. If you want control over the comparison, you can also provide a custom comparison function as the second argument."

The key here is that we are only doing a shallow comparison. So if we have a component that accepts a prop called `title` that is a string, and that string value changes for whatever reason, then the memo HOC will compare the old `title` against the new one and properly determine that it is different, and trigger a rerender.

But what happens if our component also accepts a prop `handleChange` that is a callback function?

### A brief detour into JavaScript comparisons...

In case you're not overly familiar with JavaScript, data can be categorized into two categories: primitives values (strings, numbers, booleans) and references values (Objects, Arrays, Functions, and pretty much anything that isn't a primitive).

These two categories get compared differently. When comparing one primitive against another primitive of the same type, JavaScript will check whether they have the exact same value. So if the string `breakfast` has a value of "pancakes" and the string `lunch` also has the value "pancakes" then comparing them with something like `breakfast === lunch` will result in a `true` value.

However, when we compare reference values against each other, they are compared based on their location in memory, not their underlying values. So if we define an object like so:

```
const myFriend = {
  name: "David",
  senseOfFashion: "mediocre"
}
```

... and then elsewhere within our code we define a seemingly identical object:

```
const totallyDifferentPerson = {
  name: "David",
  senseOfFashion: "mediocre"
}
```

when we compare `myFriend === totallyDifferentPerson` the JavaScript engine will actually return a `false` value despite the fact that these two objects appear to have identical entries.

The reason we receive false is that these two objects live in different parts of our computer's memory. 

If we had a component that received two props `friend` and `family` but passed our `myFriend` object as the value for both of those props, then when we compared `friend` and `family` the JS engine should return `true` since we've basically passed the same location in memory as both arguments, and so JS can see that both props actually point to the same spot in memory.

There are open source tools out there that do allow you to compare two different objects and see whether the entries within the entry are the same, rather than simply comparing the two addresses in memory, but the point is that such comparisons are not the default behavior of the JS engine.

Thus far I've used an Object as part of the demonstration, but the same result will apply to any non-primitive value.

## Back to our question about passing a callback function...

When React says it does a "shallow comparison" as part of React.memo, it means that if it receives a callback function as one of the props, if the function was redefined (which will happen if the function is being defined in some parent component and then prop drilled into our child component) then the function will always have a new place in memory. This applies even if the actual code for that function has not changed. 

As a result, React.memo will always wind up rerendering our component, making the extra work to memoize the component nothing more than unnecessary overhead.

Keep in mind that our memoization will fail for any non-primitive prop. That importantly includes the `children` prop that most developers use frequently.

## _"But can't you memoize the prop within the parent component?"_

One solution to passing non-primitive values as props into our component is to actually memoize them wherever they are defined.

So in the case of a callback, within our parent component we could memoize it like so:

```
const handleChange = useCallback((name) => {
  return name.trim();
}, [])
```

This would make it so if the parent component rerenders, we don't redefine `handleChange` and instead return the previously defined version of it, aka we return the same place in memory that it already lived in. 

Then, when we send it as a prop to our child component, React.memo() would be able to see that the address in memory hasn't changed and so it doesn't need to rerender.

Pretty cool...

...but is that something you want to pray every engineer on your team remembers to do? Do you truly believe every single human that has to interact with your codebase will remember to memoize any non-primitive values wherever they're defined so that React.memo's shallow comparison can do its job consistently?

I know the teammates that I've worked with have generally been great people but, just like me, they aren't perfect. If engineers wrote 100% perfect code 100% of the time then we wouldn't have any need for Peer Review.

## _"Well I can just write a custom comparison function so I don't need to worry about my other teammates forgetting to memoize values"_

One thing that React.memo allows is a custom callback function that can be invoked to compare the props between rerenders, rather than relying on the default shallow comparison built into the HOC.

So, in theory, you can just write a custom comparison function to handle comparing non-primitive props, rather than relying on anyone who has to utilize your component memoizing the reference values they pass as props.

There are two big issues with this approach.

1. You're getting to the point where you're adding a lot of complexity, especially if you need to compare deeply nested objects.

2. Comparisons are not free. One of the reasons React.memo relies on a shallow comparison rather than some crazy catch-all comparison is that a shallow comparison is relatively cheap to perform whereas comparing deeply nested objects is more computationally expensive. So if you define a custom comparison there's a decent chance it's going to be more expensive and you run the risk of a Pyrrhic Victory by gaining a more consistently correct comparison at the cost of worse performance.

## Seeing the big picture

As engineers, we should focus first on writing our code so that it is readable by other engineers rather than writing significantly more complex code to achieve micro-optimization.

Even if you managed to write a custom comparison function that makes determining whether to rerender your memoized component more efficient than React's default behavior, there is probably still going to be a tradeoff in readability and/or maintainability of that code down the road.

And this is already a big "if" anyways -- you're far more likely to write a custom comparison function that you think is efficient but is actually slowing things down, rather than writing a comparison that truly improves the performance of your app.

I'm not saying that you should never define a custom comparison function, but that you need to measure the performance difference between React's default behavior, React.memo's default behavior, and your customized React.memo behavior. If you can conclusively determine that you are seeing a performance benefit, you then need to decide how big that benefit is and whether it is worth the added complexity.

## Don't make assumptions

React is fast by default, so any time you start attempting to make it faster with custom logic you should really consider whether you are making things more performant, and at what cost.

React.memo exists for a reason. It can improve the performance of your components in certain circumstances. But it's also not the default behavior in React for a reason.

If you wrap every component in React.memo, there's a strong chance you'll wind up with a decrease in your performance since a decent chunk of the comparisons done for each memoized component will be unnecessary work.

At the same time, this doesn't mean you should never use React.memo. If you use the React profiler or some other tool and determine that a portion of your app is noticeably slow due to unnecessary rerenders, look at that part of your code and see whether you can wrap it in React.memo to improve the performance.

React.memo is not a one-size-fits-all solution, but it is a tool within your toolbox as a React developer that you can utilize intelligently when the situation calls for it.

Featured Post

Creating Great Pull Requests

Other Blog Posts

Expose Your Ignorance

Why not wrap everything in React.memo?