JavaScript Calculator - feedback desired

JavaScript Calculator - feedback desired
0

Hello everybody,

here’s the link to my JavaSript Calculator:

It took me quite a while to get this project done and I’m curious about your feedback!

Cool. I like how the buttons turned out. It looks good all in all, the code looks clean.

Now let me put on my “critical hat”, as if this were a pull request coming in at work…

There are a few functional problems. For example:

1. Type `1 + 2 =`. So far so good. Now press `3`. The top number turns to “6”? And then hit `=` again and it turns to “63”.
2. If I do `1 / 7 =`, the numbers overrun the container.
3. `1 / 0 =`, that gives you “infinity”, OK. But after that do `3 +` and you get weirdness in the upper number and then follow it with `3 =` and it’s “0”. This problem may be related to the first one.

Now the code (keep in mind that I’m very picky when it comes to code - I’ll try to go easy).

It’s not very “React-y” to put everything in a “god component” like this. It may not matter on something small like this, but it’s very important on bigger apps. I would have broken it up into a display and buttons section. I would have had a subcomponent for each button. I probably would have stored the button attributes in a data structure and mapped over them to create the buttons. (That last one is probably more subjective, but would result in tighter and DRYer code.) I would have had a structure like:

``````    CalcPad -|
|-- Display
|-- Buttons--|
|--Button
``````

They may not have mentioned it in FCC yet, but you don’t actually need the constructor here, you can create the state prop directly on the class:

``````class CalcPad extends React.Component {
state = {
displayInput: 0,
displayStore: ' ',
stateDecimal: false,
firstValueZero: true,
stateOperator: false,
stateEquals: false,
formula: ''
}
// ...
``````

Things like `if (this.state.displayInput=='0')` give me pause, you should be using strictly equals (`===`). It’s faster and safer and more explicit.

It is unnecessary to say `else if (this.state.stateOperator==true`, when `else if (this.state.stateOperator)` does the same thing. That is, unless you want to confirm that it is exactly equal to `true`, in which case you would want to use `===` anyway. But you don’t need that her.

This brings me to another point - the naming of variables, your boolean variables especially. I see something like stateOperator and I assume that it is an operator, probably a string. There is a convention to name boolean variables starting with “is”, “has”, “can”, or “should”. I think that isStateOperator would be more clear, or whatever fits. There is another convention that functions start with action verbs - which you do. And other things should begin with a noun (or an adjective describing it) telling you what it is, which you seem to do.

But most of that is nitpicky stuff. Good job.

And one other thing: you end up writing out your initial state twice - once for setting it the first time (constructor) and once in clearClick. It would have been DRYer to write it out in a const and just use that in both places. Plus, if you later need to change something, you only have to change it in one place. That can be a life saver on a big project.

I’ve fixed this functional problem. The other things like renaming variables and splitting the code I will do in the next weeks.

I think I will post my other too, it’s a really big help letting review the code by experts like you.

Thx

I don’t know about “expert” - we’re all just learners on different points of the path.

The color contrast of white on orange is not accessible. You can check contrast accessibility at:

https://webaim.org/resources/contrastchecker/

Thank you for this information! I will pay attention to this in future projects.

Best wishes