4
\$\begingroup\$

I have a calculator (in React) and I wonder if the way I did is is good or what I could improve. I wanted to do it like the iphone calculator (you dont only see results when you press =) and I feel like my code became a little messy. Any suggestions would be awesome! Ps. I didn't yet add more then +,-,/.*,= operations.

I also added the code on codepen:

https://codepen.io/julianyc/pen/bXwxbg?editors=1010

  const math_it_up = {
      ['+']: function (x, y) {
          console.log(x, y, typeof x, typeof y, "blaaa")
          return x + y
      },
      ['-']: function (x, y) {
          return x - y
      },
      ['*']: function (x, y) {
          console.log(x, y, typeof x, typeof y, "blaaa")
          return x * y
      },
      ['/']: function (x, y) {
          return x / y
      }
  }

  class App extends React.Component {
    state = {
          result: 0,
          operator: "",
          currentNum: "",
          show: 0
      };

      onClick = (elem) => {

          if (isNaN(elem)) {
              if (elem === "=") {
                  this.setState({
                      showResult: this.state.result
                  })
              } else {
                  this.setState({
                      operator: elem,
                      show: this.state.result ? this.state.result : null
                  })
              }

          }

          else {
              if (!this.state.currentNum) {
                  this.setState({
                      currentNum: parseInt(elem),
                      result: parseInt(elem),
                      show: elem
                  })
              } else {
                  if (this.state.operator) {
                      this.setState({
                          currentNum: parseInt(elem),
                          result: math_it_up[this.state.operator](this.state.result, parseInt(elem)),
                          show: elem
                      })
                  }

              }

          }

      };


      render() {      
          return (
              <div>
                  <Key onClick={this.onClick}/>
                  <h1>{this.state.show}</h1>
              </div>
          )
      }
  }

  const Key = ({onClick}) => {

      const renderNumbers = () => {
         const arr = [0,1,2,3,4,5,6,7,8,9];
          return arr.map((val, i) => {
              return <button key={i} name={val} onClick={(e) => onClick(e.target.name)}>{val}</button>
          })
      };

      const renderCalculationKeys = () => {
          const arr = ["+", "-", "/", "*", "="];
          return arr.map((val, i) => {
              return <button key={i} name={val} onClick={(e) => onClick(e.target.name)}>{val}</button>
          })
      };

      return (
          <div>
              {renderNumbers()}
              {renderCalculationKeys()}
          </div>
      )
  };

    ReactDOM.render(
      <App />,
      document.getElementById('root')
    );
\$\endgroup\$
  • \$\begingroup\$ ah I didnt notice, just updated it with the correct link \$\endgroup\$ – javascripting Jul 26 at 9:34
2
\$\begingroup\$

Thumbs up for putting the operators in an object, here's some improvements i would suggest :

  • There's a typo when user clicks on " = ", show instead of howResult.
  • in the math_it_up Object you don't need the [].
  • I would Rename Key to Pad for better understanding.
  • use val instead of e.target.value ( in onClick(val) ) since you have the value in there.
  • add radix parameter to parseInt like : parseInt(myVar, 10) , or use the Unary_plus +.
  • no need for the parseInt in the else of if(isNaN).
  • instead of prevState.result ? prevState.result : null you can do prevState.result || null to avoid repeating.
  • setState is asynchronous : if you need to update the state with a value that depends on the this.state, it's better to pass a callback to setState instead of an object, see this post for more details.

Her's a snippet of what the code would look like with the changes :

const math_it_up = {
  "+": function(x, y) {    
    return x + y;
  },
  "-": function(x, y) {
    return x - y;
  },
  "*": function(x, y) {    
    return x * y;
  },
  "/": function(x, y) {
    return x / y;
  }
};

class App extends React.Component {
  state = {
    result: 0,
    operator: "",
    currentNum: "",
    show: 0
  };

  onClick = elem => {
    if (isNaN(elem)) {
      if (elem === "=") {
        this.setState( prevState => ({          
          show: prevState.result
        }));
      } else {
        this.setState(prevState => ({
          operator: elem,
          show: prevState.result || null
        }));
      }
    } else {
      if (!this.state.currentNum) {
        this.setState({
          currentNum: elem,
          result: elem,
          show: elem
        });
      } else {
        if (this.state.operator) {
          this.setState(prevState => ({
            currentNum: elem,
            result: math_it_up[this.state.operator](prevState.result, elem),
            show: elem
          }));
        }
      }
    }
  };

  render() {
    return (
      <div>
        <Pad onClick={this.onClick} />
        <h1>{this.state.show}</h1>
      </div>
    );
  }
}

const Pad = ({ onClick }) => {
  const renderNumbers = () => {
    const arr = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    return arr.map(val => {
      return (
        <button key={val} name={val} onClick={() => onClick(val)}>
          {val}
        </button>
      );
    });
  };

  const renderCalculationKeys = () => {
    const arr = ["+", "-", "/", "*", "="];
    return arr.map(val => {
      return (
        <button key={val} name={val} onClick={() => onClick(val)}>
          {val}
        </button>
      );
    });
  };

  return (
    <div>
      {renderNumbers()}
      {renderCalculationKeys()}
    </div>
  );
};

ReactDOM.render(<App />, document.getElementById("root"));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.6.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.6.3/umd/react-dom.production.min.js"></script>
<div id="root"></div>

\$\endgroup\$
  • \$\begingroup\$ Thank you! I actually noticed there is also a mistake in my code, because I cannot add/divide etc numbers bigger then 9. How could I change my code so i can add etc numbers bigger then 9? \$\endgroup\$ – javascripting Jul 26 at 16:14
1
\$\begingroup\$

Reviewing your current app

Nice work so far! Calculators make educational projects because you can incrementally scale up the complexity. As the application grows, numerous design decisions in maintaining expression state need to be made and interesting parsing and equation evaluation situations arise. Here are some thoughts on the current code:

Typo

showResult: this.state.result should be show: this.state.result. This typo causes = to break.

Keep style consistent

Code with inconsistent style is difficult to read and increases the cognitive load on any humans working with the code, increasing the chances of bugs.

  • Instead of switching between snake_case and camelCase, use camelCase always in JS (class/component names are PascalCase).
  • Although semicolons are, in most cases, optional (my preference is to always include them), the code switches back and forth between including them and not.
  • Avoid goofy variable names like math_it_up. This is distracting and unclear.
  • Remove console.log calls from code before releasing it.

Separate concerns

Even though pains were taken to separate renderCalculationKeys and renderNumbers, they're virtually identical code. Both share the same overburdened onClick handler which uses nested conditionals to differentiate the actions. Conditionals are undesirable because they make state and flow difficult to reason about and have low semantic value. Using separate handlers makes it possible to eliminate this conditional and compartmentalize handler logic into distinct chunks.

Use ES6 syntax

  • math_it_up takes advantage of first-class functions and is easily extensible for new operations you might wish to add. Arrow functions can simplify this abstraction, eliminating noisy return keywords. Calling the returned function whenever code elsewhere in the class needs an evaluation is a bit of a burden; wrapping this object in an evaluate() function simplifies the calling code.
  • Use destructuring to avoid repeating this.state. If React 16.8 is available for your project, try hooks.

Additional remarks

  • The Key component is misleadingly named because it actually renders plural Keys.
  • Temporary variables like const arr = [...] which are used only once on the next line could be maped in one statement, cutting out the extra step. Even if it is kept, the name arr could be clearer as numbers or operators.
  • this.state.result ? this.state.result : null could be this.state.result || null.
  • Since the component is a calculator, consider calling it Calculator instead of App, which is usually a generic top-level container.

A rewrite suggestion

Since you've asked for the ability to add multiple numbers, you might find that the current code is a bit unwieldy due to the excessive state. There are a few ways to solve this. The approach I took is to store the expression in an array called this.state.expr. This makes it possible to use the length of the expression array to determine which buttons cause which action in a given state. This isn't a general solution should you wish to expand to longer equations, but it's a reasonable choice for supporting the current desired functionality.

A user may want the option to start a fresh expression by pressing a number key directly after = was pressed. This can be achieved with a justComputed flag. I've also used parsing to avoid leading zeroes like 007. I left Infinity as the outcome of division by zero and chose to ignore numerical overflow.

Here's a version that addresses the above points in addition to supporting multi-digit numbers:

class Calculator extends React.Component {
  state = {expr: ["0"], justComputed: false};
  
  evaluate() {
    const {expr} = this.state;
    return {
      "+": (x, y) => x + y,
      "-": (x, y) => x - y,
      "/": (x, y) => x / y,
      "*": (x, y) => x * y,
    }[expr.splice(1, 1)](...expr.map(e => +e));
  }
  
  handleEq = e => {
    if (this.state.expr.length === 3) {
      this.setState({
        expr: ["" + this.evaluate()], 
        justComputed: true
      });
    }
  };

  handleNum = e => {
    const {justComputed, expr} = this.state;
    const num = e.target.name;

    if (justComputed) {
      this.setState({
        expr: [num], 
        justComputed: false
      });
    }
    else if (expr.length === 2) {
      this.setState({expr: expr.concat(num)});
    }
    else {
      this.setState({
        expr: expr.concat(+(expr.pop() + num) + "")
      });
    }
  };
  
  handleOp = e => {
    const {expr} = this.state;
    const op = e.target.name;
    
    if (expr.length === 1) {
      this.setState({
        expr: expr.concat(op), 
        justComputed: false
      });
    }
    else if (expr.length === 2) {
      this.setState({expr: expr.pop() && expr.concat(op)});
    }
    else {
      this.setState({expr: ["" + this.evaluate(), op]});
    }
  };
  
  render() {
    const {expr} = this.state;
    return (
      <div>
        <Keys 
          handleNum={this.handleNum} 
          handleOp={this.handleOp} 
          handleEq={this.handleEq} 
        />
        <h1>{expr.length < 3 ? expr[0] : expr[2]}</h1>
      </div>
    );
  }
}

const Keys = ({handleNum, handleOp, handleEq}) => 
  <div>
    {[..."0123456789"].map(e =>
      <button
        key={e}
        name={e} 
        onClick={handleNum}
      >{e}</button>
    )}
    {[..."+-/*"].map(e =>
      <button
        key={e}
        name={e} 
        onClick={handleOp}
      >{e}</button>
    )}
    <button
      key="="
      name="="
      onClick={handleEq}
    >=</button>
  </div>
;

ReactDOM.render(<Calculator />, document.getElementById("root"));
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.6.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.6.3/umd/react-dom.production.min.js"></script>
<div id="root"></div>


Next steps

Some features you might consider adding:

  • Show the current expression as it's being built in a separate element.
  • Support negative numbers.
  • Add a . decimal button. Ensure that states such as 1.2.3 are disallowed.
  • Add undo/redo/history support and/or CE, C and Backspace buttons.
  • Add CSS and create an attractive UI.
  • Make it possible to add long expressions such as 5+3*5/-2-7.
  • Add functions such as sqrt, sin, cos, tan, log, etc.
  • Add support for parenthesis.
  • Support big integers and scientific notation.
\$\endgroup\$
  • \$\begingroup\$ Be sure to also handle division by zero :) \$\endgroup\$ – AleksandrH Jul 26 at 22:21

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.