1

I have an array [ 0.00, 1.00, 2.00, 3.00 ] and 4 input fields to edit each number in the array. Right now I have an onChange function for each number. How do I make this code more efficient?

class Form extends React.Component {
    constructor(props) {
      super(props);
      this.state = {
        value: [ 0.00, 1.00, 2.00, 3.00 ]
      };
    }
  
    field0 = (e) => {
        this.setState({ 
          'field0' : e.target.value,
        },() => {
          this.setState({ value: 
            [ 
              Number(this.state.field0), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[1]), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[2]), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[3]),
            ]
          });
        });
      }
    
      field1 = (e) => {
        this.setState({ 
          'field1' : e.target.value,
        },() => {
          this.setState({ value: 
            [ 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[0]), 
              Number(this.state.field1), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[2]), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[3]),
            ]
          });
        });
      }
    
      field2 = (e) => {
        this.setState({ 
          'field2' : e.target.value,
        },() => {
          this.setState({ value: 
            [ 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[0]), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[1]),
              Number(this.state.field2),  
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[3]),
            ]
          });
        });
      }
    
      field3 = (e) => {
        this.setState({ 
          'field3' : e.target.value,
        },() => {
          this.setState({ value: 
            [ 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[0]), 
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[1]),
              Number(this.state.value.map(function(v){ return v.toFixed(2); })[2]),
              Number(this.state.field3), 
            ]
          });
        });
      }
  
    render() {
      return (
        <div>
          <input name="field0" onChange={this.field0} value={this.state.value.map(function(v){ return v.toFixed(2); })[0]} /><br /> 
          <input name="field1" onChange={this.field1} value={this.state.value.map(function(v){ return v.toFixed(2); })[1]} /><br />  
          <input name="field2" onChange={this.field2} value={this.state.value.map(function(v){ return v.toFixed(2); })[2]} /><br />  
          <input name="field3" onChange={this.field3} value={this.state.value.map(function(v){ return v.toFixed(2); })[3]} />
         <p>
            {this.state.value.map(function(v){ return v.toFixed(2); })[0] + ', ' +
             this.state.value.map(function(v){ return v.toFixed(2); })[1] + ', ' +
             this.state.value.map(function(v){ return v.toFixed(2); })[2] + ', ' +
             this.state.value.map(function(v){ return v.toFixed(2); })[3]}
          </p>
        </div>
      );
    }
  }
  
  ReactDOM.render(<Form />, document.getElementById('root'));
Jay Singh
  • 15
  • 3
  • Since it's basically the same method for each field (just the field is different), you can refactor it to use one method and just pass the name as the prop name and the value. As for the order of the array, maybe you can also pass the index of the field so you can replace the item at that specific index. – dork Apr 08 '21 at 05:29
  • Check my answer – Rashed Rahat Apr 08 '21 at 05:44

4 Answers4

2

Your code isn't very DRY (Don't Repeat Yourself). It can be improved.

  1. Use a single change handler for all the inputs that use the index for the array element you want to update.
  2. You can map the state to the inputs and the mapped and joined display.
  3. There is no need really to reformat the state after each update, just store the data and do the formatting when you are rendering the JSX.

Code

class Form extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      value: [0.0, 1.0, 2.0, 3.0]
    };
  }

  // Consume index and onChange event
  changeHandler = (index) => (e) => {
    // map previous state to next state
    this.setState((prevState) => ({
      value: prevState.value.map((val, i) =>
        i === index ? e.target.value : val
      )
    }));
  };

  render() {
    return (
      <div>
        {this.state.value.map((val, i) => (
          <React.Fragment key={i}>
            <input
              name={"field" + i}
              onChange={this.changeHandler(i)} // <-- pass index to curried handler
              value={Number(val).toFixed(2)} // <-- format for display
            />
            <br />
          </React.Fragment>
        ))}

        <p>
          // map the state to formatted values then join into string for display
          {this.state.value.map((v) => Number(v).toFixed(2)).join(", ")}
        </p>
      </div>
    );
  }
}

Demo

Edit in-react-js-how-to-get-value-from-multiple-input-fields-when-one-of-them-is-cha

Drew Reese
  • 165,259
  • 14
  • 153
  • 181
1

You can use the spread is operator to construct the array instead the use a map function. You can see how it works here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

0

Here is the better way:

import React from "react";

class Form extends React.Component {
    constructor(props) {
    super(props);
    this.state = {
      value: [0.0, 1.0, 2.0, 3.0]
    };
  }

  changeHandler = (index) => (e) => {
    let { value } = this.state;
    value[index] = e.target.value;
    this.setState({ value });
  };

  render() {
    return (
      <div>
        {this.state.value.map((val, i) => (
          <div key={i}>
            <input
              type="text"
              onChange={this.changeHandler(i)}
              value={Number(val).toFixed(2)}
            />
            <br />
          </div>
        ))}

        <p>{this.state.value.map((v) => Number(v).toFixed(2)).join(", ")}</p>
      </div>
    );
  }
}

export default Form;

Visit to see CodeSandbox live demo

Happy coding :)

Rashed Rahat
  • 2,357
  • 2
  • 18
  • 38
  • This approach is the one that resonated with me the most. Thank you for your help! – Jay Singh Apr 08 '21 at 17:37
  • @Jayjay note that [this mutates the current state, which is an anti-pattern in React.](https://stackoverflow.com/q/37755997/1218980) See the other answers to avoid mutating the state. – Emile Bergeron Apr 14 '21 at 23:28
0

this would be a better approach. but however, there are issues with changing the floating-point input in your idea.

import React from "react";
import ReactDOM from "react-dom";

class Form extends React.Component {
  constructor(props) {
   super(props);
   this.state = {
     values: [0.0, 1.0, 2.0, 3.0]
   };
  }

changeValue(idx, value) {
  this.setState(prevState => ({
      values: prevState.values.map((v,i) => idx == i ? parseFloat(value) : v)
    }));
}

renderInputs() {
  const inputs = this.state.values.map((v, i) => {
    return (
      <div>
        <input
          name={"feild" + i}
          onChange={e => this.changeValue(i, e.target.value)}
          value={v.toFixed(2)}
        />
      </div>
    );
  });

  return inputs;
}

renderValues() {
  return this.state.values.map(v => v.toFixed(2)).join(", ");
}

render() {
  return (
    <div>
      {this.renderInputs()}
      <p>{this.renderValues()}</p>
    </div>
  );
}
}

ReactDOM.render(<Form />, document.getElementById("root"));

you can find it here sample code