DEV Community

7 code smells in your React components

Anton Gunnarsson on November 09, 2020

A growing collection of things I consider code smells in React components. Too many props Incompatible props Copying props into state Returning J...

Read full post

JP Erasmus • Nov 12 '20

Excellent list of code-smells! I fully agree with these.

The most subtle one is the functions returning JSX inside a component. That is essentially the definition of a React component, i.e. "function-returning-JSX", which should be a clue to abstract it outside the parent.

The one that I personally learnt the hard-way was too many things in a single useEffect.

Andrew Vitale • Dec 8 '20

This is a TIL for me, no doubt. I largely work with Angular, but use React for custom components for our geospatial mapping integrations. My React is littered with little JSX methods. Hopefully, a year from now I'll look back at that code and have to plug my nose, too 👃😉

Peter Ellis • Nov 11 '20

Thank you, that's a neat collection of arguably fishy-smelling patterns.

There is one I haven't quite made up my mind about, and that's something like this:

function SomeComponent(props) {
    const someMoreProp = // calculate some extra stuff
    return <>
          ....
         <OtherComponent {...props} moreProp={someMoreProp}/>
         ...
     </>;
}

I do find this pattern quite useful when I've got different variants of a component (for example a text and a number input component), but I want to expose it as just a single GenericInput component.

But it doesn't feel right.

Anton Gunnarsson • Nov 12 '20 • Edited

Thanks!

If it works it's works, so to speak :) Your example is close to writing a Higher-order component but it depends on the use case. A lot of HOCs use cases however (just as render props) have been replaced by hooks instead, and should probably be the default moving forward unless you have a special use case ^^

Sylvie Fiquet • Nov 13 '20 • Edited

Hi! I really enjoyed your article. The options pattern will help with a component of mine that didn't feel right.

I think the reducer example needs to be edited though:

the dispatch type for selectItem is 'reset', I believe it should be 'selectItem' to match the reducer?
shouldn't the case for selectItem do the same thing as the function? isOpen and inputValue won't change on their own. Or am I missing something?

    case "selectItem":
      return {
        ...state,
        isOpen: false,
        inputValue: action.payload.name,
        selectedItem: action.payload
      }

Anton Gunnarsson • Nov 13 '20

Thank you Sylvie and good catch! A bit too fast with the copy/paste it seems! :D Updated it now! 🙏

Peter Kellner • Nov 12 '20

Hmmm. any other references for this being bad? "Returning JSX from functions". For your trivial example, I'd agree, but sometimes it's not that crazy to do w/o creating separate files. What about them makes them hard to read?

Anton Gunnarsson • Nov 13 '20

Yeah, for me it's about reducing the complexity of all components and minimizing context switching but sometimes it might be the right thing to use a function. In my experience though it's better to either inline the JSX or move it to another component in the vast majority of cases. (Altough that component might still live in the same file if it's tightly coupled).

Vasile Stefirta 🇲🇩 ✈️ 🇺🇸 • Nov 13 '20

Hey Anton, really good tips! 👏

I've been focusing a lot myself on writing not just code "that works" but also clean and maintainable. The "future you" and anyone else touching that code will definitely thank you for that 🙂

Anton Gunnarsson • Nov 13 '20

Thanks! Very much agree with that sentiment!

Steve Huckle • Nov 12 '20 • Edited

The only one I wasn't convinced by was returning JSX. The trivial example you give, sure.

But is it wrong to use a function to create large amounts of JSX programmatically because the JSX you want to render depends on large amounts of varying data (perhaps you've fetched thousands of values from an API)? In that situation, I'd create some JSX elements containing the fetched values in a loop (or equivalent) and push them into an array. Once the array contained all my values, I'd set some state variable to that array, then render that. So not exactly returning JSX, but not that different! Is that wrong? If so, what's the alternative?

Anton Gunnarsson • Nov 13 '20

It's never completely wrong if it works :)

In the example you give I wouldn't store the JSX in state, but rather the data needed to create them and then map then data to JSX in render. If there's a lot of complex logic involved in deciding which JSX should be rendered it can often be alleviated by creating smaller components, using composition or other patterns such as render props or custom hooks.

Thanks for questioning! 🙌

Steve Huckle • Nov 13 '20

Hmmm - that's probably right - but the state is actually in Redux, I just use the component state as a placeholder for render. I'll have a look at the component in question and see what I can do with it in another iteration.

Good stuff!

Anton Gunnarsson • Nov 13 '20

Ah yeah give it a try, good luck!

SimonAking • Nov 13 '20

Thank the author for sharing. I have gained a lot.

Can I translate it into Chinese and send it to my blog?
I will indicate your information and original address at the beginning of the article.

Anton Gunnarsson • Nov 13 '20

Thanks!
Sure, that would be great! Feel free to send me the link afterwards! :)

SimonAking • Nov 14 '20

Ok, after I finish the translation, I will send you the link, so stay tuned~

SimonAking • Nov 15 '20

It took me a day to get it done. The Chinese translation is here. Thank you again for sharing.

tomotoes.com/blog/7-code-smells-in...

Avi Block • Nov 30 '20

Looking at the props of this component we can see that all of them are related to what the component does, but there's still room to improve this by moving some of the components responsibility to its children instead:

but now this component is no longer reusable, and the consumer of it has to make sure it's set up correctly.

Lesley van der Pol • Nov 13 '20

Very interesting post! I'm glad you reminded me of useReducer, I'll definitely be rewriting a thing or two in my client's codebase using it.

Segun Abisagbo • Nov 14 '20

this was a great article.

Learnt some new practices.

love the if () return <Component /> style.

reads better then ternary

Nattapon Pondongnok • Nov 10 '20

😻 Its awesome

Anton Gunnarsson • Nov 10 '20

Thanks! 🙌

charles hollins • Feb 10 '21

Excellent, thanks!

Rafael Nunes • Nov 18 '20

Great article 🥳

Lucian Naie • Nov 13 '20

your applicationForm example is meh. it makes me think you want to keep yourself busy, which I hope is not the goal here

Anton Gunnarsson • Nov 13 '20

I'm not sure what you mean with "want to keep yourself busy", could you elaborate? :)

I'm inclined to agree that the example isn't the best though, might update that if I come up with something clearer. Composition however is a fantastic tool for reducing complexity, increasing testability etc!

Lucian Naie • Nov 13 '20

I meant to create more work and billing hours, also to keep it simple versus over-engineering it. I know, I know, it depends on the context.

Anton Gunnarsson • Nov 13 '20

Ah, I see. Although I work as a consultant today I would do the same if I weren't getting paid for it :) Composition is great, even if you shouldn't go wild and try to compose everything ^^

Arif RH 🎮 • Nov 13 '20

excellent article, thanks for sharing!

Steve Huckle • Nov 12 '20

There's some good stuff in there, Anton - thanks! I really like the reducer example (is this the end of Redux?). And you can have more than one useEffect! Who knew? :)

Anton Gunnarsson • Nov 13 '20

Thanks!

While I personally don't use redux nowadays (I usually reach for Overmind when I need it), there's still a lot of people who use it successfully. Especially with Redux Toolkit which removes so much of the friction people are used to with redux.

ogrotten • Nov 11 '20

Excellent info. Good article. Thanks for posting.

Raja Jaganathan • Nov 11 '20

Thanks for sharing 👍

Lars • Nov 12 '20

I love the loading, error, finished example. I tend to use a lot of boolean states

Blessing Hirwa • Nov 30 '20

Wow! This really helped me make my codes more readable. Thanks a lot Anton.

Nadav Lebovitch • Nov 28 '20

Good article! Definitely learned something.

aRTUR • Nov 13 '20

In some projects I'm using React, but I need to learn a more solid base for I don't make amateur mistakes like this "large useEffect". Can someone share the way to become a react's king?