Hi I have following code :
const handleChange = ({ selectedItem }) => {
if (selectedItem?.value) {
if (name === FieldNames.SecurityQuestion1) {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
} else if (name === FieldNames.SecurityQuestion2) {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
}
}
};
Here, I am trying to refactor this without using the if else and switch condition. So I tried with the ternary operator but it also does not work:
selectedItem?.value ? (name === FieldNames.SecurityQuestion1) && resetField(FieldNames.FirstAnswer, '');
But this does not work as it is a wrong syntax. Can any one help me with this ?
Thanks.for the help.
CodePudding user response:
If you just want to avoid nested ifs, why not put it in this way?
const handleChange = ({ selectedItem }) => {
if (!selectedItem?.value) {
return;
}
if (name === FieldNames.SecurityQuestion1) {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
} else if (name === FieldNames.SecurityQuestion2) {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
}
};
or this:
const handleChange = ({ selectedItem }) => {
if (selectedItem?.value && name === FieldNames.SecurityQuestion1) {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
} else if (selectedItem?.value && name === FieldNames.SecurityQuestion2) {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
}
};
CodePudding user response:
I'm not sure I can recommend refactoring your code that way but... since you ask, here is a way to do it without if/else/switch.
const condition0 = selectedItem?.value;
const condition1 = name === FieldNames.SecurityQuestion1;
const condition2 = name === FieldNames.SecurityQuestion2;
const doThen = () => {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
};
const doElse = () => {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
};
condition0 && (
!(condition1 && (doThen() || true))
&& condition2 && doElse()
);
CodePudding user response:
I don't recommend refactoring, because it's perfectly readable code and it seems to work fine, but I think this would solve your problem:
selectedItem?.value ? ((name === FieldNames.SecurityQuestion1) && resetField(FieldNames.FirstAnswer, '')):null
CodePudding user response:
In my opinion you are refactoring the wrong thing. Obviously you are looking at your code and see that it is a bit of a mess with two places where the code seem to be repeated but not quite:
if (name === FieldNames.SecurityQuestion1) {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
} else if (name === FieldNames.SecurityQuestion2) {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
}
Realise that the ternary operator is just an if
in expression context instead of a statement. Converting it will result in exactly the same mess with less readable syntax.
If you REALLY want to know the correct ternary expression for the code you have it looks exactly the same:
selectedItem?.value ?
name === FieldNames.SecurityQuestion1 ?
resetField(FieldNames.FirstAnswer, '')
:
resetField(FieldNames.ConfirmFirstAnswer, '')
:
name === FieldNames.SecurityQuestion2 ?
resetField(FieldNames.SecondAnswer, '')
:
resetField(FieldNames.ConfirmSecondAnswer, '');
The above is the correct way to abuse the ternary operator and get the same result. However, notice that it is still the same code and the same mess. And if you put the :
in the wrong place it would be a bug and produce the wrong result!! The braces {}
used by the if
statement makes understanding and debugging the code much easier because it clarifies when a condition starts and when it ends.
The way to refactor this is to realize that Question1
and Question2
have common data. So structure it that way:
const FieldNames = {
Security: {
First: {
Question: '...',
Answer: '...',
Confirm: '...'
},
Second: {
Question: '...',
Answer: '...',
Confirm: '...'
}
}
}
Now you can refactor it like this:
const handleChange = ({ selectedItem }) => {
if (selectedItem?.value) {
for (let f of ['First', 'Second']) {
if (name === FieldNames.Security[f].Question) {
resetField(FieldNames.Security[f].Answer, '');
resetField(FieldNames.Security[f].Confirm, '');
}
}
}
};
This reduces duplicated code and makes your code DRY (and hence more compact) without reducing readability.
CodePudding user response:
You can try this:
const handleChange = ({ selectedItem }) => {
const answerIndex = selectedItem?.value ? (name === FieldNames.SecurityQuestion1 ? 'FirstAnswer' : name === FieldNames.SecurityQuestion2 ? 'SecondAnswer' : null) : null;
if (answerIndex) {
resetField(FieldNames[answerIndex], '');
resetField(FieldNames[`Confirm${answerIndex}`], '');
}
}
CodePudding user response:
const handleChange = ({ selectedItem }) => {
if (selectedItem?.value && name === FieldNames.SecurityQuestion1) {
resetField(FieldNames.FirstAnswer, '');
resetField(FieldNames.ConfirmFirstAnswer, '');
}
if (selectedItem?.value && FieldNames.SecurityQuestion2) {
resetField(FieldNames.SecondAnswer, '');
resetField(FieldNames.ConfirmSecondAnswer, '');
}
};
This is how I imagine a YouTuber I've seen who didn't like else
statements would do it and addresses you concerns about the nested if/else
statement.
<opinion> I rather think it's an improvement in terms of clarity and maintainability but as to whether it's the best way to set up the code - no comment. </opinion>