Reputation: 397
I have this code that happens quite often in React (handling input from user).
const handleEndDateChange = ( e ) => {
if ( e.target.name === "month-selected" ) {
setChosenEndDate( {
...chosenEndDate,
month: e.target.value
} );
}
if ( e.target.name === "year-selected" ) {
setChosenEndDate( {
...chosenEndDate,
year: e.target.value
} );
}
if ( e.target.name === "day-selected" ) {
setChosenEndDate( {
...chosenEndDate,
day: e.target.value
} );
}
};
My question is: Is there a nicer and more elegant way to write this? Not only for React, but in general, when it comes to functions that handles these types of actions.
I have recently read "Clean Code" book, and in it it mentions 2 things I keep thinking about.
1: You can tell someone is junior by the overuse of IF statements. 2: A function should only do ONE thing (This function as i showed you, is doing 2 things. First, determining the input, and then setting state).
Upvotes: 1
Views: 310
Reputation: 4739
The 3 things this function does at once (but shouldn't do) are:
name
from eventname
to period (year, month, day)Reasons for functions to be doing one thing is mainly them being reusable and easy to argue about. In your case, for example, you can't reuse it to set chosen end date programmatically, because there is no event
generated. If anything goes wrong, you can't tell whether the problem is in the mapping of names, or assigning values, or reading names from event targets. If you change your event target's name you will have to go to this function and change them here as well. And better hope you don't forget to do it
Consider this example instead
const handleEndDateChange = (period, value) => {
setChosenEndDate({
...chosenEndDate,
[period]: value
})
}
In this case it does only one thing: writing value for respective period. Logic of assigning periods to onChange events belongs to your components themselves, e.g.
// somewhere in render()
<MonthItem onChange={e => handleEndDateChange('month', e.target.value)} />
<YearItem onChange={e => handleEndDateChange('year', e.target.value)} />
<DayItem onChange={e => handleEndDateChange('day', e.target.value)} />
In this case onChange prop for your items knows which period it's responsible for, and it is responsible for mapping it's name (e.g. "month-selected"
, if you still need it) to key in your end date ("month"
)
Upvotes: 1
Reputation: 17858
The most elegant way to do is like this:
const handleEndDateChange = e => {
setChosenEndDate({
...chosenEndDate,
[e.target.name]: e.target.value
});
};
Your input fields must have name attribute.
Another alternative with destructuring:
const handleEndDateChange = e => {
const {name, value} = e.target;
setChosenEndDate({
...chosenEndDate,
[name]: value
});
};
Upvotes: 6
Reputation: 2132
const handleEndDateChange = ( e ) => {
setChosenEndDate( {
...chosenEndDate,
[e.target.name]: e.target.value
});
}
in this case: "month-selected"
- "year-selected"
- "day-selected"
are [event.target.name]
It works? If not please comment and we together find a solution.
Upvotes: 3
Reputation: 9769
Your input field must have name attribute to identify uniquely.
const handleEndDateChange =(e)=> {
const {name, value} = e.target;
setChosenEndDate({
...chosenEndDate,
[name]: value
});
};
Upvotes: 1