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')
);
-
\$\begingroup\$ ah I didnt notice, just updated it with the correct link \$\endgroup\$javascripting– javascripting2019年07月26日 09:34:54 +00:00Commented Jul 26, 2019 at 9:34
2 Answers 2
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 ofhowResult
. - in the
math_it_up
Object you don't need the[]
. - I would Rename
Key
toPad
for better understanding. - use
val
instead ofe.target.value
( inonClick(val)
) since you have the value in there. - add
radix parameter
toparseInt
like :parseInt(myVar, 10)
, or use the Unary_plus+
. - no need for the
parseInt
in theelse
ofif(isNaN)
. - instead of
prevState.result ? prevState.result : null
you can doprevState.result || null
to avoid repeating. setState
is asynchronous : if you need to update the state with a value that depends on thethis.state
, it's better to pass a callback tosetState
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>
-
\$\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– javascripting2019年07月26日 16:14:39 +00:00Commented Jul 26, 2019 at 16:14
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 noisyreturn
keywords. Calling the returned function whenever code elsewhere in the class needs an evaluation is a bit of a burden; wrapping this object in anevaluate()
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 pluralKeys
. - Temporary variables like
const arr = [...]
which are used only once on the next line could bemap
ed in one statement, cutting out the extra step. Even if it is kept, the namearr
could be clearer asnumbers
oroperators
. this.state.result ? this.state.result : null
could bethis.state.result || null
.- Since the component is a calculator, consider calling it
Calculator
instead ofApp
, 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.
-
\$\begingroup\$ Be sure to also handle division by zero :) \$\endgroup\$Aleksandr Hovhannisyan– Aleksandr Hovhannisyan2019年07月26日 22:21:58 +00:00Commented Jul 26, 2019 at 22:21