I have this piece of code

// This is onChange event
function nameChangeHandler(e, field = 'Name') {
        const currentName = e.target.value;

        if (currentName.length >= 10) {
            if (!errors.some((x) => x.field == field)) {
                setErrors((state) => [
                    ...state,
                    {
                        field,
                        errorMessage:
                            field  
                            ' should be between 3 and 10 characters long',
                    },
                ]);
            }
        } else if (currentName.length <= 3) {
            if (!errors.some((x) => x.field == field)) {
                setErrors((state) => [
                    ...state,
                    {
                        field,
                        errorMessage:
                            field  
                            ' should be between 3 and 10 characters long',
                    },
                ]);
            }
        } else {
            setErrors((state) => state.filter((x) => x.field !== field));
        }
    }

This part of the code is redundant

  if (!errors.some((x) => x.field == field)) {
                setErrors((state) => [
                    ...state,
                    {
                        field,
                        errorMessage:
                            field  
                            ' should be between 3 and 10 characters long',
                    },
                ]);
            }

My question is if place it inside an useCallback hook. Will it be a good practice or it's stupid move. Refactored code should be looking something like this:

const errorSetter = useCallback((field) => {
        if (!errors.some((x) => x.field == field)) {
            setErrors((state) => [
                ...state,
                {
                    field,
                    errorMessage:
                        field   ' should be between 3 and 10 characters long',
                },
            ]);
        }
    }, []);

function nameChangeHandler(e, field = 'Name') {
        const currentName = e.target.value;

        if (currentName.length >= 10) {
            errorSetter(field)
        } else if (currentName.length <= 3) {
            errorSetter(field)
        } else {
            setErrors((state) => state.filter((x) => x.field !== field));
        }
    }

I tried adding it in useCallback and it works fine. All though, I'm new to React and don't yet know the best practices.

CodePudding user response：

If the component you're using nameChangeHandler on is memoized, there's an argument for memoizing nameChangeHandler (rather than errorSetter); details in my answer here. But you could make errorSetter a utility function that lives outside the component, like this:

function errorSetter(field, setErrors) {
    setErrors((errors) => {
        if (errors.some((x) => x.field == field)) {
            return errors;
        }
        return [
            ...errors,
            {
                field,
                errorMessage: field   " should be between 3 and 10 characters long",
            },
        ];
    });
}

const YourComponent = () => {
    const nameChangeHandler = useCallback(function nameChangeHandler(e, field = "Name") {
        const currentName = e.target.value;

        if (currentName.length >= 10) {
            errorSetter(field, setErrors);
        } else if (currentName.length <= 3) {
            errorSetter(field, setErrors);
        } else {
            setErrors((state) => state.filter((x) => x.field !== field));
        }
    }, []);

    // ...
};

Notice how the utility function doesn't rely on anything it closes over, it just uses the field and setErrors it's passed; and the only thing nameChangeHandler uses that it closes over is setErrors, which is guaranteed to be stable. So nameChangeHandler can be memoized via useCallback (or useMemo, or useRef, which are other ways of doing it).

By making nameChangeHandler stable, you avoid unnecessary re-renders of the component you pass it to — if that component is memoized.

CodePudding user response：

It can be simplified using or operation like:

function nameChangeHandler(e, field = 'Name') {
    const currentName = e.target.value;

    if (currentName.length >= 10 || currentName.length <= 3) {
        if (!errors.some((x) => x.field == field)) {
            setErrors((state) => [
                ...state,
                {
                    field,
                    errorMessage:
                        field  
                        ' should be between 3 and 10 characters long',
                },
            ]);
        }
    } else {
        setErrors((state) => state.filter((x) => x.field !== field));
    }
}

to reduce redundant code. I don't think useCallback is what you need here. The only usage of useCallback I can imagine here is to wrap the whole function in useCallback to avoid re-rendering the element you're passing it to.