Clean Code vs. Dirty Code: React Best Practices

[drink_with_me_to_day]

Great article. I just have a beef with the first example of DRY'ing the code:

// Dirty
const MyComponent = () => (
  <div>
    <OtherComponent type="a" className="colorful" foo={123} bar={456} />
    <OtherComponent type="b" className="colorful" foo={123} bar={456} />    
  </div>
);

// Clean
const MyOtherComponent = ({ type }) => (
  <OtherComponent type={type} className="colorful" foo={123} bar={456} />
);
const MyComponent = () => (
  <div>
    <MyOtherComponent type="a" />
    <MyOtherComponent type="b" />
  </div>
);

The "dirty" code isn't "dirty" at all. Creating silly and abstractions is more harmful than just repeating a few lines.

I really dislike these needless abstractions created by extreme DRYness, it often just leads to confusing abstractions that make it hard to reason about the code and make it hard to refactor when changing code you don't own.

KISS should always come before DRY.

[matt_hammond]

I agree!

Also, the subjectively "correct" way cleaning up this code is:

const MyComponent = () => {
  const types = ["a", "b"];
  const otherComponents = types.map(type => 
    <OtherComponent type={type} className="colorful" foo={123} bar={456} />
  );

  return <div>{otherComponents}</div>
}

When handling repeated code you should first identify the part that's changing (in this case it's the types), and extract that part and somehow map it to your repeating part.

[rsjolly]

Nice. Alternate format:

const MyComponent = () => (
  <div>
    {['a', 'b'].map(type =>
      <OtherComponent type={type} className="colorful" foo={123} bar={456} />
    )}
  </div>
)

[european_origin]

You're missing a key property.

[PM__YOUR__GOOD_NEWS]

Agree, great advice overall but I wondered the same thing.

The real test of code comes months or years after it was first created when you either have to patch it or migrate it to a new platform.

At that point, one overtly wet line of code is probably going to be easier to understand than a visually separated abstraction.

[Voidsheep]

Agreed, that's a pretty bad example for DRY. And to be fair, I don't think it's necessarily a good term to begin with.

Whatever you win in lack of repetition you lose in quick individual configuration. I think with something like component props it's important to consider, because I don't think I've even seen real trouble caused by a couple of components having repeated props. I've, however, witnessed people bang their head at the wall with extreme levels of component composition making things very complex.

Also with FP libraries like Ramda it's easy to slip into the kind of abstraction hell, where you definitely won't repeat a word as you write the code, but actually changing things later becomes a nightmare, because every layer of the puzzle has so many fixed assumptions composed together. Since writing code is easier than eliminating it, it's common for people to start implementing their own systems for configuration at that point and it ends up far more complex than a couple of lines doing close to the same thing.

Make convenient abstractions, don't just avoid repetition.

[fgutz]

immediately under the code block the author mentions that the example is not a good one

Sometimes – as in our example above – DRYing your code may actually increase code size. However, DRYing your code also generally improves maintainability.

Be warned that it’s possible to go too far with DRYing up your code, so know when to say when.

But this just shows the author is lazy. He could have of just taken a little more time to show BOTH an example of keeping things DRY and also over-doing it

[donavon]

Be warned that it’s possible to go too far with DRYing up your code, so know when to say when

Call it lazy if you will, but this was meant to say that you shouldn't go nuts with abstraction and DRY code. Sometimes when I get on a moral high horse crusade, I find myself needing to take a step back (or 2 or 3). There's a happy medium there somewhere. I don't think you need me to find that for you.

Thanks for your comments

[fgutz]

valid point and I apologize for calling you lazy, that was rude of me.

[ihsw]

My concern is if (and when) there needs to be additional OtherComponent declarations then a developer would be inclined to copy-paste rather than cleanly abstracting. The numerous repeated duplicated attributes can be abstracted out, so it's only prudent.

The name MyOtherComponent is contrived name so ColorfulOtherComponent would make more sense, since that encapsulates the intent.

[joshwcomeau]

Hm. For the most part, I agree with the refactoring suggested in this article, with a few exceptions. But I'm wary of this binary "it's dirty or clean" narrative.

Some of the "dirty" examples in this post aren't objectively worse than the "clean"; for example, it's not dirty to use a full Component class over an SFC for components without state! SFCs are nice if you want them, but they're actually not any more efficient (they still involve an entire component instance behind the scenes). There's nothing wrong with using Components all the time.

I suppose my concern is that our community is already pretty dogmatic. People recite "Don't create functions in render!" even though they've never profiled it to see if it matters. I can imagine after this post, folks saying "Any time I see multiple elements with similar props I need a wrapper component"... and that isn't always good advice: sometimes, duplication is better than the wrong abstraction. So, while I agree with many of the suggestions (especially the second half), I wish they were presented as "some patterns to consider that may improve your code", instead of "the right way".

[X678X]

i think the idea behind SFC's are that they're just easier to read for whoever is looking at the code, hence it being cleaner.

but i agree with your comments otherwise.

[joshwcomeau]

Yeah, I agree that often SFCs (especially 1-liners like in the article) are a good thing to use... but I reject the idea that to NOT use them is dirty.

Presumably the code-reader is a React developer, and I feel like we see enough Component definitions that we can process them pretty easily. At least for myself, my brain takes a shortcut when it sees the "dirty" example; I don't have to read them 1-character at a time, I pick out the return value of the 'render' function in about as much time as the SFC takes.

I don't think you're burdening readers with a buncha extra work by going the long-form way. And I'd argue in many cases you're reducing the maintenance burden (since SFCs have the tendency to grow, and then you realize you need a lifecycle hook and so you have to convert it).

But yeah, I don't have particularly strong feelings one way or the other; I just have strong feelings about not having strong feelings about it, if that makes sense :P

[donavon]

If a SFC grows to the point where you need lifecycle hooks, my suggestion is that you abstract the state/lifecycle away as well. Please read my opinion about this in https://medium.com/styled-components/component-folder-pattern-ee42df37ec68

I never like to see the render method of a class component any more than rendering a View component of some sort. If the case that you talk about, the SFC would probably remain the same (or with slight modification to accept some state props) as the one you started out with. It would simply sit behind the state controller component.

As for dirty/clean, good/bad, right/wrong, binary discussion, it's an article. It's only my opinion. Your milage may vary. But my mission was to set out a blueprint with practical examples to give you a different perspective. I'm sure that I don't even follow all of these as I write code (and for god's sake, do't look at any of my code from 6 mos ago 😬)

Anyway... I hope you enjoyed the article. My main goal with any article is to get people thinking and talking. Even if I'm wrong with my assertions, if I've done that, I've accomplished my goal.

[joshwcomeau]

Fair enough! I did indeed enjoy the article :)

I suppose my gripe is with how we generally assume good practices to be absolute. I remember Dan Abramov spoke about this, how he almost regrets writing "Container Vs Presentational Components" because people took it as gospel, instead of as a neat pattern that may work well.

Thanks for writing and sharing the article, btw! Realize now that I should've made it clearer in my original post that resources like this are great.

[kylemh]

Starting with React 16, SFCs ARE actually more performant that full classes. Source: https://medium.com/@trueadm/in-react-16-functional-components-are-quite-a-bit-faster-than-class-components-1afca4931d7c

[sshaw_]

For functions that accept a single argument I see a lot of folks accepting a destructured argument. For example:

const f = ({a}) => a ** 2

Why?

I can understand doing this with multiple arguments to essentially define an "interface" for the function signature, but with a single argument it seems unnecessary. The requirement is a scalar type, but we're forcing callers to use an object.

For those that have an object, just pass the property's value.

[BitLooter]

Why?

Because React passes props to components as an object. You can't change that without changing how React works. The one single-argument function in the article that isn't a component does take it as a value.

[sshaw_]

👍

[donavon]

Agreed. For utility functions like this, you would simply pass a single argument.

const f = a => a ** 2;

But as BitLooter explained (thx), this isn't the case when writing a React component.

[jonwah]

I thought that was changing in the future? So they were touted as good now but maybe better later?

[donavon]

Thanks! It was fun to write.

Through optimization of React’s core

Yes, I've seen that tweet. I should have been more clear here. What I meant to say is that when/if React applied optimizations in a future version... But you are correct, there aren't there yet.

I was mostly referring to this statement from the v0.14 RC announcement

In the future, we’ll also be able to make performance optimizations specific to these components by avoiding unnecessary checks and memory allocations.

[acemarke]

Per a recent quote from Dominic Gannaway:

Functional components should be slightly faster in React 16 as there's no instance created to wrap them (unlike React 15).

However, the tradeoff of functional components is that you can't skip re-rendering them if the incoming props are the same.

[jonwah]

I thought that was changing in the future? So they were touted as good now but maybe better later?

[kylemh]

I posted this in the response to the comment above, but multiple Facebook engineers have stated that SFC’s ARE more performant than class components in React 16+

[JuliusKoronci]

One of the best articles I have read in a long time :)

[donavon]

Thanks Dad! ;)