JavaScript Calculator - feedback desired

Code Feedback
1

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!

2

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.

3

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.

4

Thank you for your feedback.

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

5

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

6

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

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

7

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

Best wishes