CODEWITHSUNDEEP
javascriptreactjs
Etsh
Etsh

Reputation: 166

Is it okay make event handling functions return a function?

Hello I am new to react and I was wondering if I could return a function in a event handler instead of passing to the element a arrow function with parameter? Like this:

deletePersonHandler(person) {
    return () => {
        const persons = [...this.state.persons];
        const index = persons.indexOf(person);
        persons.splice(index, 1)
        this.setState({ persons });
    }
}

and on component render:

function Person(props) {
    function Children() {
        if (props.person.children) {
            return <p>{props.person.children}</p>
        }
        else return null;
    }

    return (
        <div className="person">
            <p>Hello I am {props.person.name}, and I am {props.person.age} years old.</p>
            <input value={props.person.name} type="text" onChange={(event) => { props.change(event, props.person.id) }} />
            
            {/* Over Here!! */}
            <button onClick={props.onDelete(props.person)}>Remove</button>

            {Children()}
        </div>
    )
}

I tested the code and it runs fine but I'm asking if there are any setbacks or consequences? and if so what are the alternatives? Here is the full code's repository on my Github's account: https://github.com/EtshD1/react-components

Upvotes: 0

Views: 100

Answers (2)

Gabriel Donadel Dall&#39;Agnol
Gabriel Donadel Dall&#39;Agnol

Reputation: 709

You could set the name of the button as the id of the person you want to delete that way you can get the id from the event.target.name and avoid return an anonymous function. it would look like this:

deletePersonHandler(event) {
    const personId = Number(event.target.name) //only use Number() if the personId is a numer
    const persons =  this.state.persons.filter(({id}) !== personId)
    this.setState({ persons });
}

And on the component you would use:

function Person(props) {
function Children() {
    if (props.person.children) {
        return <p>{props.person.children}</p>
    }
    else return null;
}

return (
    <div className="person">
        <p>Hello I am {props.person.name}, and I am {props.person.age} years old.</p>
        <input value={props.person.name} type="text" onChange={(event) => { props.change(event, props.person.id) }} />
        
        {/* Over Here!! */}
        <button name={props.person.id} onClick={props.onDelete}>Remove</button>

        {Children()}
    </div>
)}

You should always avoid passing parameters to a function at the render() level as this will cause reconciliation

Upvotes: 1

JMadelaine
JMadelaine

Reputation: 2964

Technically yes. Sure you're creating 1 extra unnecessary function, but JS is super efficient and you'll never hit an upper limit on number of functions able to be created.

However, by executing a function in the onClick prop, you're making your code a lot less readable. Developers are used to seeing a function definition for that prop, and by calling a function, the first thing I think is "that should be a definition, not execution", so my inclination would be to think there is a bug with your code, when there isn't.

You've also named the function onDelete, as in "this function gets called on delete", but it doesn't it gets executed immediately and the returned function gets called on the click event.

Part of programming is the ability to not just write working code, but write readable code. Your code is going to be read by other developers, and the more obscure and unnecessary code you write, the harder you're making it for the people reading it.

So in conclusion, is it fine to do this? Technically yes, but semantically, no.

Upvotes: 2

Related Questions