Every developer wants to write clean and concise code for his apps so that he does not face any issues while debugging them.
But still, sometimes we get tempted or honey-trapped and make some pretty common mistakes that are not recommended or are categorized as anti-pattern or bad practices in React which should be avoided at all costs.
Otherwise, we will have to face some serious performance issues later in the development process.
In this blog, we will discuss some ReactJS Bad Practices that developers do, and how to avoid them.
Index
- Using Index as the key in the map function
- Polluting Render method by using Anonymous functions
- Using Nested Components
- Nesting Ternary Operator in Render
- Not Destructuring Props
- Prop Drilling
- Not cleaning up Event Listeners
- Using Inline CSS
- Using Divs everywhere
Let’s start…
1. Using Index as the Key in the map() function
The map() function is used to print all the elements of an array into a new array by calling a function for each element.
In react, the map() function requires a key to distinguish between each element and to detect their exact changes.
According to the official documentation, ‘A key is a special string attribute you need to include while creating lists of elements. Keys help React identify which items have changed, are added, or are removed. Keys should be given to the elements inside the array to give the elements a stable identity.’
Problem
Using the index of an array as the key for the map() function is not recommended as there can be a change in the order of elements if we perform any operation like addition, deletion, etc. on them.
Because of this, React will not be able to detect the exact changes in the state which can cause some serious performance issues.
Example
Suppose we have a list of 5 elements with key as the index
<ul>
<li key={1}>Milk</li>
<li key={2}>Eggs</li>
<li key={3}>Food</li>
<li key={4}>Bread</li>
<li key={5}>Sausage</li>
</ul>;
Now, in this case, there is a state change like adding a new item, deleting an item, etc., React just iterates over each list in both the cases and updates the React DOM with only the state that has some changes in it (Virtual DOM concept).
Let’s say, we have added an item at the end of the list. As there is no change in the order of the items, React will only render once to add the extra item at the end.
<ul>
<li key={1}>Milk</li>
<li key={2}>Eggs</li>
<li key={3}>Food</li>
<li key={4}>Bread</li>
<li key={5}>Sausage</li>
<li key={6}>Butter</li>
</ul>;
But what if we have to add an item at the beginning or in the middle of the list.
This time, there will be a change in the order of each item, and because of that React will re-render all the elements again and not the one that has been added.
<ul>
<li key={1}>Butter</li>
<li key={2}>Milk</li>
<li key={3}>Eggs</li>
<li key={4}>Food</li>
<li key={5}>Bread</li>
<li key={6}>Sausage</li>
</ul>;
Solution
This can be very easily avoided by using a unique id. Let’s take the same example again but this time the key will have a unique value for every item.
<ul>
<li key={"1a"}>Milk</li>
<li key={"2b"}>Eggs</li>
<li key={"3c"}>Food</li>
<li key={"4d"}>Bread</li>
<li key={"5e"}>Sausage</li>
</ul>;
Now even if we add elements at the beginning or end, we won’t face an issue since keys are different and it has nothing to do with the index of the array.
Since, React tracks all list items with their key attribute, after adding a new element it would not re-render the previous list items.
2. Polluting Render Method by using Anonymous Functions
To understand this, let’s take an example
import React from "react";
const App = () => {
const handleClick = () => {
console.log("You Clicked???????");
};
return <button onClick={() => handleClick()}>Click me</button>;
};
export default App;
There is no problem with this code, and it’s also giving us our desired output as shown.
Then, why is it not recommended?
Problem
The problem with this syntax is that a different callback is created each time the Button renders.
In most cases, this is fine. However, if this callback is passed as a prop to lower, there will be many extra re-renders.
What it means is, by passing an anonymous function, React will always re-render itself since it receives a new anonymous function as a prop which it is unable to compare to the previous anonymous function as both of them are anonymous.
Solution
We can use the binding in the constructor or using the class fields syntax, or simply pass the function as an event handler to avoid this sort of problem.
By doing this we are telling React that nothing is changed, so that unnecessary re-renders can be avoided.
3. Nested Components
React has given us the ability to divide one huge component into countless small components, and link them with the help of props which has made our code cleaner and more understandable.
But sometimes, we get tempted and avoid the use of props. We declare both parent and the child component in the same component as shown
import React, { useState } from "react";
const Main = () => {
const [name, setName] = useState("Ateev");
const Sub = () => {
return <h1 className="p-5">Hello {name}, I am the child of Mr & Mrs Khana</h1>;
};
return (
<>
<Sub />
</>
);
};
export default Main;
There is nothing wrong with defining both our parent and the child component under the same hood, and the app will also work fine, but there will be some serious performance issues with our app.
Problem
We will receive performance issues because every time our Main Component gets rendered, the Sub Component also gets rendered, and this goes on for infinity
Solution
By passing props we can solve this issue very easily as now we are telling React until and unless there is a change in the prop, we don’t want the Sub Component to be rendered.
Top comments (6)
There are several weak points in the article. I think some examples are unfit to illustrate their respective issue properly. Sometimes a code example is missing when it would be reasonable to show one. The shown code snippets are poorly formatted. You state personal preference as established wisdom. And the mixup of function based and class based components in the second section makes that section's conclusion rather incoherent.
To be more precise:
mapfunction, the list is static and therefore the keys are actually just fine, and not even needed to be precise. You state performance issues as a result, but that is not correct. This has nothing to do with performance. The problem really is - if the list changes - that the DOM update will be incorrect. If the user deletes an element in the middle of the list, the DOM update will instead throw away the last entry. A direct result of using the index as the key. And as others have pointed out: the solution to the problem should show how the user can just take any unique field of the entry data to serve as a key. Like so:The issue is that functions that are defined inside the render function are a completely fresh instance every time the render is run, and comparing them by reference will determine that they are not the same. As others have pointed out, you should have shown in you solution section, that either
useCallbackhooks can be used to make the reference stable (for as long as the dependencies are stable) or that you could just pull up the function definition from inside your render function and instead have it top-level in your module code. But your current solution section states: "binding in the constructor or using the class fields syntax" but both concepts are not applicable in a function based component.I think your solution part is missing necessairy examples. If you show what the wrong code would look like, you should also show what the right code would look like.
That is rather confusing. It sounds like we will get an infinity render loop here. Well, that is not the case. The only issue with defining components inside the parent's render function is that the child will be unmounted and re-mounted whenever the parent is rendered. If the child uses
useStatehooks those will be reset to their initial values. I agree that usually we should not define sub-components inside render, unless we have a very specific need for actually defining 'dynamic' components.Here - as in the section before that one - I think your solution part is missing necessairy examples. If you show what the wrong code would look like, you should also show what the right code would look like.
I clicked the link to your page under "Continue Reading" and read the other sections also.
"Nesting Ternary Operator in Render": In my opinion the example code you show does not get clearer by replacing the ternary with a chained if-else syntax.
To the contrary. The ternary was actually better.
All in all this is rather all about personal preference. I would not have included it in an article about "Bad Practices and Anti Patterns".
"Not Destructuring Props": Again this is personal preference and I would have excluded it from this article. It can sometimes even make sense to have the
propsobject: if you want to support arbitrarydata-*oraria-*attributes you would want to do something like this:Enough for now.
You really think the nested ternary is better? SHEESH
I agree this doesn't belong in list of "best practices / anti patterns" but just no, that ternary stuff is garbage.
The issue with keys isn't quite as you've said here, it won't rerender everything just because you used the index as a key. In fact in the example there would be no problem at all. The issue is with "other things related to the item" for instance if you render components that themselves have hooks or state then it will fall out of sync with the items that were rendered last time leading to some very hard to identify bugs if you rearrange items or insert into the list. For example, the hooks for the first item will be the values from the item rendered first last time when you insert at the start of the list. If you use a string id then it is smart enough to figure all of this out and still provide the old hook values from an item with the same key at that level of the hierarchy.
-> Nesting Ternary Operator in Render.
This one in particular makes me want to throw up
Exactly
At least the points listed here are all rather poor examples of both the problem and the solution.
useCallback, plain and simple.