DEV Community

loading...

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...
pic
Editor guide
Collapse
jperasmus profile image
JP Erasmus

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.

Collapse
vitale232 profile image
Andrew Vitale

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 👃😉

Collapse
almostconverge profile image
Peter Ellis

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}/>
         ...
     </>;
}
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
awnton profile image
Anton Gunnarsson Author • 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 ^^

Collapse
sfiquet profile image
Sylvie Fiquet • 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
      }
Enter fullscreen mode Exit fullscreen mode
Collapse
awnton profile image
Anton Gunnarsson Author

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

Collapse
pkellner profile image
Peter Kellner

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?

Collapse
awnton profile image
Anton Gunnarsson Author

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).

Collapse
dev_nope profile image
Vasile Stefirta 🇲🇩 ✈️ 🇺🇸

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 🙂

Collapse
awnton profile image
Anton Gunnarsson Author

Thanks! Very much agree with that sentiment!

Collapse
glowkeeper profile image
Steve Huckle • 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?

Collapse
awnton profile image
Anton Gunnarsson Author

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! 🙌

Collapse
glowkeeper profile image
Steve Huckle

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!

Thread Thread
awnton profile image
Anton Gunnarsson Author

Ah yeah give it a try, good luck!

Collapse
segebee profile image
Segun Abisagbo

this was a great article.

Learnt some new practices.

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

reads better then ternary

Collapse
blocka profile image
Avi Block

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.

Collapse
lesleyvdp profile image
Lesley van der Pol

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.

Collapse
tomotoes profile image
SimonAking

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.

Collapse
awnton profile image
Anton Gunnarsson Author

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

Collapse
tomotoes profile image
SimonAking

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

Collapse
tomotoes profile image
SimonAking

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

Collapse
naijab profile image
Nattapon Pondongnok

😻 Its awesome

Collapse
awnton profile image
Collapse
charles66982918 profile image
charles hollins

Excellent, thanks!

Collapse
peaonunes profile image
Rafael Nunes

Great article 🥳

Collapse
lnaie profile image
Lucian Naie

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

Collapse
awnton profile image
Anton Gunnarsson Author

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!

Collapse
lnaie profile image
Lucian Naie

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.

Thread Thread
awnton profile image
Anton Gunnarsson Author

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 ^^

Collapse
cetholl_ profile image
Arif RH 🎮

excellent article, thanks for sharing!

Collapse
glowkeeper profile image
Steve Huckle

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? :)

Collapse
awnton profile image
Anton Gunnarsson Author

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.

Collapse
ogrotten profile image
ogrotten

Excellent info. Good article. Thanks for posting.

Collapse
rajajaganathan profile image
Raja Jaganathan

Thanks for sharing 👍

Collapse
larsolt profile image
Lars

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

Collapse
nadavl profile image
Nadav Lebovitch

Good article! Definitely learned something.

Collapse
blessingartcreator profile image
Blessing Hirwa

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

Collapse
marchiartur profile image
aRTUR

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?

Collapse
davidjo24054760 profile image
David Johnson

Great article and a nice overview of the code-smells. Thanks.

Collapse
joycecoletti profile image
Joyce Coletti

Good post, thanks for sharing. Splitting the code according to its intent is always pleasant. In the multiple locations of our program, we will reuse the very same code.

Collapse
jameslarson310 profile image
James Larson

Thanks for posting. I learned a lot.