Refactoring Chronicles: Conditional Rendering and getting rid of the Wrapper Div

If you have been working with React I bet you found yourself often in the situation where you have to conditional render some components.
During a recent code-review I notice something inside a Form Component, where we needed to display the LastUpdatedBy field, only if available in the data.

The original code was this:

const AddLastUpdatedBy = ({record}) => (
<div>
 {record.updatedBy && <DisabledInput
 source="updatedBy" label="Updated By"
/>}
</div>
)

There is nothing really bad in this code, but we could make it slightly better.

The first improvement is getting rid of the Wrapper Div that we don't need at all.

Why? Does it harm? Well, as the germans would say Jein (Yes & No): It is not a big deal - in the end it is just a small little empty additional Div in your DOM, but it could cause some unpredictable behaviour if you are using FlexBox or Css Grid. (read a more detailed explanation here and here)

So what is the alternative?

React.Fragment

If you replace the Wrapper <div> with a <React.Fragment> the behaviour stays the same but you will not find any additional div in your Dom.

What is even better ( although a bit weird to see the first times) is using the more concise '<> </>' to wrap the component you need to conditionally render.

const AddLastUpdatedBy = ({record}) => (<>
 {record.updatedBy && <DisabledInput
 source="updatedBy"
 label="Updated By"
/>}
</>)

Still, I believe this snippet can be written more nicely.

Could we use a ternary operator to conditionally render the component?
Yes we could:

const AddLastUpdatedBy = ({record}) => record.updatedBy ? (<DisabledInput source="updatedBy" label="Updated By"
/>
    ) : (   <></>  )

but I doubt this would be a more readable improvement.
And the last line reminds me some weird ascii emoji

Unfortunately, we can't just write :

 const AddLastUpdatedBy = ({record}) => record.updatedBy && 
(<DisabledInput source="updatedBy" label="Updated By"
/>) 

because we would get an error in the condition when nothing has to be rendered:

But the message is very clear, just return null.

const AddLastUpdatedBy = ({record}) => record.updatedBy && (
<DisabledInput source="updatedBy" label="announcements.fields.updatedBy"/>) || null

The outcome is exactly the same as the Fragment <> option, but in my opinion is more concise and readable: it clearly tells us we want the DisableInput or Nothing

Hope it helps.

---

Photo by Bench Accounting on Unsplash

Comments

[Daniel Brady]

I doubt this would be a more readable improvement

I'm glad to find someone else giving "readability" feedback in code reviews!

In 5 years, I've gone from a "how concisely/cleverly can I write this?" approach to coding, to a "how well can someone with no context read what I've written?" approach. And it I've experienced nothing but good things from making that shift.

[Davide de Paolis]

often "readability" it is just a matter of habit. I remember when I moved to Javascript at the beginning I found very hard reading the Arrow functions
consider this

function multiplyByTwo(a) {
     return a*2;
}

versus :

const multiplyByTwo = a => a*2

Back then I would argue that the more verbose way was more understandable, right now i prefer the concise version.

It contains only what matters, no clutter.

But I completely agree that ( unless when there are performance issues or other reasons ) it is much better to write small (tiny) methods that can be compose together rather than a big one with lots of stuff in it.
Just think about a single for loop which filters and sorts and concatenate and manipulate data vs 4 different functions all concatenated. sure it's one iteration vs 4 but code is more readable and more unittestable.
And as you said reducing the cognitive load, and the amount of context you need to have to understand a piece of code is the most important thing.