Bad React JS Practices

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

1. Using Index as the key in the map function
2. Polluting Render method by using Anonymous functions
3. Using Nested Components
4. Nesting Ternary Operator in Render
5. Not Destructuring Props
6. Prop Drilling
7. Not cleaning up Event Listeners
8. Using Inline CSS
9. 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.

Continue Reading.

Comments

[Martin]

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:

• "Using Index as the key in the map function": Your code example doesn't really show code that would have an issue. There is not usage of the map function, 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:

const food = [
  { name: 'banana', diet: 'vegan' },
  { name: 'beans', diet: 'vegan' },
  { name: 'blueberries', diet: 'vegan' },
  { name: 'butter', diet: 'vegetarian' },
  { name: 'beemster cheese', diet: 'vegetarian' },
  { name: 'beef', diet: 'omnivore' },
];
<ul>
  {food.map(item => (
      <li key={item.name}>{`${item.name} : ${item.diet}`}</li>
    ))}
</ul>

• "Polluting Render Method by using Anonymous Functions": That statement isn't really getting at the gist of the issue: > 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.

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 useCallback hooks 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.

• "Nested Components": You state: > our Main Component gets rendered, the Sub Component also gets rendered, and this goes on for infinity

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 useState hooks 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 props object: if you want to support arbitrary data-* or aria-* attributes you would want to do something like this:
  

const Foo = (props) =>  {
  const dataProps = util.extractDataProps(props);
  const ariaProps = util.extractAriaProps(props);

  return (
    <div className="foo" {...dataProps} {...ariaProps}>...</div>
  );
}

Enough for now.

[Daniel Ko]

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.

[Mike Talbot ⭐]

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.

[Nwanguma Victor]

-> Nesting Ternary Operator in Render.
This one in particular makes me want to throw up

[Ateev Duggal]

Exactly

[FJones]

At least the points listed here are all rather poor examples of both the problem and the solution.

1. The keys in the "solution" still have no discernible connection to the actual item, they're again just iteratively generated. It also doesn't really show how that works in practice with actual modified data.
2. A function component as the example, and then the suggestion is to use constructor-based binding? The solution here is to use useCallback, plain and simple.
3. Again, the problem here is lack of memoization. The sub component can very well be declared within the main component (though indeed it ideally should be using props). There's valid reasons for it (e.g. dynamically switching out component implementations from a local map), but in those cases the rerender issues are resolved or reduced by using memoization to ensure the sub component remains the same unless its declaration changes.