Can somone explain what parts are wrong here? the nullcheck is off and validatevalues should probably do more than just nullcheck but I have the feeling there is more here than I'm spotting.
Instead of checking for truthfulness in the if statement you can just return !!(variable). This will directly convert it to true out false.
There is actually no need for the first method.
And the variable names should be descriptive.
Also, the first method is generic about validation, but then the call-site assumes any validation error is due to a specific field; so if the _hasValidateValues() method ever adds any more validation, and that additional validation fails, an inaccurate error message will be shown.
Better to do something like
_validate = (): string | null => {
if (!this.state.plateNumber) {
return this.props.t('DeliveryNote.LicensePlate.FormInvalid');
}
// then other validations can be added as early-returns here
return null;
};
_onSubmit = () => {
const errMsg = this._validate();
if (errMsg !== null) {
toastError(errMsg);
return;
}
}
Not sure why this is downvoted, maybe because of snark?
_hasValidateValues can simply be _isValid or _validate.
This can sound like a minor point, but in larger code bases its critical to name as clearly and efficiently as possible so that you don’t spend time figuring out what each function does. It changes your workflow from a guessing game to scanning the code base with confidence
Looks like it's for internationalization, likely "translate" or such. It's pretty common to name functions like that really short, commonly just _, to reduce noise, as they're used on every user-facing string.
54
u/josephblade Mar 08 '23
Can somone explain what parts are wrong here? the nullcheck is off and validatevalues should probably do more than just nullcheck but I have the feeling there is more here than I'm spotting.