3
\$\begingroup\$

I have just started playing with React today and built a simple Calculator app. Would someone please be able to advise on steps I could take to improve my code?

Ideally, I want ideas on how to properly use state, and how to refactor out my use of the eval function.

Would appreciate any feedback on how you would have done this differently as well :)

import React from 'react';

class Calculator extends React.Component {
  constructor(props){
      super(props);
      this.state = {
        prev: [],
        total: []
      };
  }

  inputDigit(int) {

    var newArr = this.state.total.slice();
    newArr.push(int);
    this.setState({total: newArr});

  }

  addition(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "+");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  minus(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "-");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  divide(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "/");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  multiply(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "*");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;
  }

  clearInput() {

    this.setState({total:[],prev:[]});

  }

  sum(currNum) {

    var store = this.arrToNum(currNum);
    var newArr = this.state.prev.slice();
    newArr.push(store);
    console.log(newArr.join(" "));
    var sum = eval(newArr.join(" "));

    this.setState({total:[sum]});

  }

  arrToNum(arr) {

    var test = Number(arr.join(''));

    return test;

  }

  makePretty(num) {

    var newNum = this.arrToNum(num);
    var formatNum = newNum.toLocaleString();

    return formatNum;

  }

  render() {
    return(
      <div className="Calculator">
        <div className="Screen">
          <p>{this.makePretty(this.state.total)}</p>
        </div>
        <div className="KeyPad">
          <div className="row">
            <button className="square" onClick={() => this.inputDigit(7)}>7</button>
            <button className="square" onClick={() => this.inputDigit(8)}>8</button>
            <button className="square" onClick={() => this.inputDigit(9)}>9</button>
            <button className="square" onClick={() => this.addition(this.state.total)}>+</button>
          </div>
          <div className="row">
            <button className="square" onClick={() => this.inputDigit(4)}>4</button>
            <button className="square" onClick={() => this.inputDigit(5)}>5</button>
            <button className="square" onClick={() => this.inputDigit(6)}>6</button>
            <button className="square" onClick={() => this.minus(this.state.total)}>-</button>
          </div>
          <div className="row">
            <button className="square" onClick={() => this.inputDigit(1)}>1</button>
            <button className="square" onClick={() => this.inputDigit(2)}>2</button>
            <button className="square" onClick={() => this.inputDigit(3)}>3</button>
            <button className="square" onClick={() => this.multiply(this.state.total)}>*</button>
          </div>
          <div className="row">
            <button className="square" onClick={() => this.inputDigit(0)}>0</button>
            <button className="square" onClick={() => this.inputDigit(".")}>.</button>
            <button className="square" onClick={() => this.sum(this.state.total)}>=</button>
            <button className="square" onClick={() => this.divide(this.state.total)}>/</button>
          </div>
          <div className="row">
            <button className="square full" onClick={() => this.clearInput()}>CLEAR</button>
          </div>
        </div>
      </div>
    );
  }
}

export default Calculator;

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Oct 24 '17 at 14:24
  • \$\begingroup\$ @Phrancis Thankyou! Me too :) \$\endgroup\$ – Jack Oct 24 '17 at 14:36
1
\$\begingroup\$

You have a fair amount of repitition:

  addition(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "+");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  minus(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "-");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  divide(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "/");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

  multiply(prevNum) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , "*");
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;
  }

I would instead add an additional parameter op and rewrite it as:

  operation(prevNum, op) {

    var store = this.arrToNum(prevNum);
    var newArr = this.state.prev.slice();
    newArr.push(store , op);
    var string = newArr.join(" ");
    this.setState({total:[] , prev: [string]});

    return prevNum;

  }

Or something along those lines.

Furthermore, once you have done this, this.arrToNum(prevNum); is only called once in your entire code. I would just get rid of arrToNum all together and just call Number(prevNum.join('')).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.