Top 7 things I check most in PRs as a Frontend engineer
In my time as a frontend developer, I’ve noticed that I end up reviewing the same errors in Pull Requests. Here are some of the most common ones I’ve come across.
1. Tricky layouts
Until FlexBox came along, CSS layouts were a complicated thing, and that’s why all the webs in the 90’s ended up looking the same. A header at the top, menus on the sides, and in the middle, a main section where the content went (poorly structured).
Now that we have FlexBox (one of my favorite CSS properties), sometimes we think it’s a cure-all, that it’s good for absolutely everything. To a hammer, everything looks like a nail.
CSS Grid arrived to solve many of the problems that frontend developers were still facing despite all the advantages of FlexBox. Often, we insist on using Flex for a certain layout when Grid can be much more interesting.
For example, one of the most underrated properties for creating frontend layouts is grid-template-areas. Take a look at its documentation and you'll see how it simplifies layout creation.
Here are some resources for you to learn more about the topic: 😊
2. Really bad naming
Just because you understand a function right now doesn’t mean you’ll understand it a month from now. Even less so if it is read by a teammate.
Naming is one of the most critical things when we develop. Bad naming means spending more time figuring out what certain parts of the application do, which makes us less productive and causes more frustration.
If you’re concerned about your lines getting too long, use a formatter like Prettier and reach a consensus within the team.
// ❌
const elements = await getElementsFromGroup();
// ✅
const groupElements = await getElementsFromGroup();
// ❌
const getDate = () => {...}
// ✅
const getCurrentISODate = () => {...}
// ❌
const selected = selectedElement.id === element.id;
const focus = focusElement.id === element.id;
// ✅
const isSelected = selectedElement.id === element.id;
const isFocus = focusElement.id === element.id;3. Inline variables
Sometimes, we try to simplify our code so much that we end up seriously impair its readability. The issue with lengthy files, classes, or functions isn’t necessarily because you’re leaving blank lines between your statements; it might be a problem with separating responsibilities (as discussed in the previous two sections).
To enhance the readability of your codebase, I recommend extracting as many constants as possible. Your project will become much more semantic, easier to read, and less prone to human errors.
// ❌
if (date > Date.now()) {...}
// ✅
const isFutureDate = date > Date.now();
if (isFutureDate) {...}
// ❌
<Button disabled={cart.elements.length === 0} />
// ✅
const isShoppingCartEmpty = cart.elements.length;
<Button disabled={isShoppingCartEmpty} />If you found this helpful or enjoyable, … 👏👏! Your claps are appreciated and keep me motivated!
4. Unused code
Most of the time, when we refactor code, we tend to forget to review the old codebase we wanted to replace.
To keep projects as clean as possible, don’t forget to remove all those files, functions, classes, hooks, or constants that got scattered throughout the project and are likely to be forgotten.
I recommend using one of the inspection tools provided by your IDE or installing a plugin to assist you with this task.
5. Magic literals
Sometimes, we think it might be a good idea to have literals dispersed throughout our codebase, what we commonly call magic numbers, but they can also be strings or booleans.
These values are typically used for default values or configurations, but it’s important to give them meaning.
// ❌
const [zoom, setZoom] = useState(100);
// ✅
const DEFAULT_ZOOM = 100;
const [zoom, setZoom] = useState(DEFAULT_ZOOM);
// ❌
const createUser = (name) => ({
name,
isPremium: false
})
// ✅
const DEFAULT_IS_PREMIUM_USER = false;
const createUser = (name) => ({
name,
isPremium: DEFAULT_IS_PREMIUM_USER
})6. Incorrect use of Testing Library
Testing Library is the library that has saved the most computers from being thrown out of the window. Testing interfaces is now much easier, but if you’re not too experienced, there’s one thing that can be tedious: rendering and the asynchrony problems that come with it.
When to use act() and when not to use it? When to use findBy, getBy or queryBy? Should I always use waitFor to avoid problems?
Kent C. Dodds, the creator of this library, wrote an article in which he described some of the most common mistakes we tend to make when using Testing Library. No matter how skilled you are at using the library, you’re bound to be surprised by some of Kent’s comments.
// ❌
const { getByRole } = render(<Form />);
const button = getByRole("button");
// ✅
render(<Form />);
const button = screen.getByRole("button");7. Typos
Please, if your IDE doesn’t have a spell-check tool, install a plugin that performs this function (even if English is your native language).
Your team members will thank you. 🙂
Now, seriously. The names we give to variables, functions, or classes are often challenging to read because we can’t add spaces to separate the different words. There are some naming conventions like “snake case” or “kebab case” where typos are more easily detected, but before imposing casing conventions on the team, install a plugin that helps you detect typos.
// ❌
const youAreAnAwesmePerson = "🙂";
// ✅
const youAreAnAwesomePerson = "🙂";BONUS! Check your own PR
When we start developing a new user story, our initial idea of it is often quite different from what it will be when we finish.
As we progress in development, we encounter situations that weren’t covered in the acceptance criteria. Therefore, it’s quite normal for our code to pivot as we realize these details that were initially overlooked.
Sometimes, we think the best developer to review a Pull Request is the one with the most time on the project or the most senior developer. But I believe the person who can review a PR best is the one who code it. Why?
- 👍 They know the task and its acceptance criteria the best.
- 👍 They are most familiar with the functions/classes/components they created.
- 👍 They can quickly spot code that’s no longer in use.
- 👍 They are the ones who best realise how things could be done better.
However, it’s crucial to be honest with oneself. Otherwise, it’s a waste of time.
Still, it’s always a good practice to have another colleague take a look to catch things you might have missed. Or if you prefer, you can use other tools like Pair Programming. It’s up to you.
I’d love to hear your thoughts on this! What do you think? Feel free to drop a comment below and share your perspective with me! 💬
