Javascript Calculator project with own logic completed!

Link to my JS Calculator Project

I would love for people to check on my most recent project and opinions are welcome, even by newbies.

Some details: Ive coded my own logic for the calculations and did not copy the example project or outside sources, which im very happy about. Its quite extensive and might be rusty, but its mine^^. The code as a whole utilizes often conditional statements and regular expressions. There are also additional features from the ones present in the example project, like backspace button, or sound effect as well as keyboard interaction, inspired by the previous project(The Drumpad Machine). You can use your num keyboard!

Hey @Sylvant, this is a very nice job. Of course I like to tinker and try to break things so I’ve got a few observations :slight_smile:

  • When I click the sound button the first time I get a sound (sounds like a quick high-hat) but then I never get a sound again :frowning:
  • I do like that I can use the keyboard to type in numbers, but I can’t do everything with the keyboard. I can’t seem to figure out how to use the clear button and when I hit the / key my browser opens a search box instead. UPDATE: I just figured out that if you do Shift+c it clears, but it is not obvious so you’ll need to let people know how to do that. The / issue still exists.
  • I am getting an Infinity result for the following: 1/1*0. I’m not sure if this is expected but normally I would expect the 1/1 to happen first and then the *0.
  • When I do get the Infinity result then everything stops working (even the Clear button). I am seeing a JS error in the console, so that should hopefully make it easy to figure out.
1 Like

All in all it looks good. I find the colors a little odd and I’m not sure what the sound-effect is for, but I’m an old fuddy-duddy. And maybe you’re just having fun - don’t worry about it.

Putting on my nitpick hat and scanning through the code, adding to bbsmooths excellent review…

let nums= ['zero','one','two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine']
// ... other code
//transforming button values into objects
nums=nums.map((num, n)=>new MyObj(num, `${n}`))

Why not just declare the object that you want? Why declare it as an array that you don’t want and map it to an array that you do want? Furthermore, you end up with:

[
   { id: "zero", content: "0" }
   { id: "one", content: "1" }
   // ...
]

Why not just something like:

{ 
  zero: 0,
  one: 1,
  // ...
}

I don’t know, maybe it will get clunkier with an object because it can’t be mapped as easily, but still, you should create the data that you need. I could make similar comments about operators and actions.

You have way too many comments. I know, when you start out, they tell you that good coders put lots of comments. No, good coders write code that rarely needs a comment. For example:

    return <div id='main'>
      {/* The Display */}
      <MyDisplay content={input}/>
      {/* Calculator Keyboard */}
      <div id='keyboard'>
        {/* Main Action Buttons */}
        {actions.map(obj=><MyButton {...obj} myclass='actions' myclick={()=>this.input(obj.content)}/>)}
        {/* Operators Buttons */}
        {operators.map(obj=><MyButton {...obj} myclass='operators' myclick={()=>this.input(obj.content)}/>)}
        {/* Digit Buttons */}
        {nums.map(obj=><MyButton {...obj} myclass='nums' myclick={()=>this.input(obj.content)}/>)}
        <audio id='my-sound' src='https://s3.amazonaws.com/freecodecamp/drums/Cev_H2.mp3'/>
      </div>
    </div>

Every one of those comments are unneeded because the line that comes after them makes it clear what it is.

For:

      {/* Calculator Keyboard */}
      <div id='keyboard'>

It is obvious that it is the keyboard because of the id.

For:

        {/* Main Action Buttons */}
        {actions.map(obj=><MyButton {...obj} myclass='actions' myclick={()=>this.input(obj.content)}/>)}

I don’t need the comment I know that they are buttons because of the component to which it is being mapped and I know that they are the action buttons because that is the array being mapped.

And this for example:

    this.state= {
      input: '0', //holds whats rendered on the display
      sound: true //controls if sound is turned on or off
    }

The first comment is redundant, and the second one would be not needed with a better variable name. What about, “isSoundOn”? There is a common convention that boolean variables start with “is” (although some further allow “can”, “has”, “should” or things like that). A good variable name tells you instantly what that variable is.

If you choose variable names well and organize well, you don’t need many comments (and sometimes none). Too many comments can be a bad thing - they clutter the code, they allow devs to feel OK about cryptic code, and since comments often don’t get maintained they may end up causing more confusion than clarity. The goal should be to write code that reads like a story and doesn’t need any comments. That being said, we do occasionally need a comment, but should only be when the code’s function would not immediately apparent to a dev looking at your code.

There are a couple of stylistic things, like you wouldn’t need a constructor if you used arrow functions for your methods (so no need to bind this) and your initials state was set directly as a class property:

class Calculator extends React.Component {
  state= { input: '0', sound: true };

  // ...

    keyupEvent = e => {  
      let key= e.key
    // ...

They’re still teaching React the “old way” but this is what you’ll see irl. And people, are using classes less and less, preferring function components with hooks, but it’s cool if you’re not there yet. Don’t worry about it too much - I’m just mentioning it fyi.

But still, it looks good, good job. Have fun on the next project.

1 Like

You got a bunch of good feedback already.

I just wanted to comment on the My prefix naming “convention” you got going on. It isn’t helpful prefixing identifiers with My and it serves no purpose. It also just makes it look like that is all you did and the rest isn’t “your” code (at least that is what I often see people do with code) so I would avoid it.

2 Likes

I wouldnt expect anything less of you :slight_smile:
Thanks for pointing out those flaws. The button which switches sound on and off has the flaw, when you turn it off, it plays sound, when you turn it on, it doesnt, so you can be deceived weather it works or not. I had to hardcode it to work properly so i let it slip away.
Im aware some keyboard functions are not intuitive. The Shift+C i didnt event know^^. Ive made so Esc can work as clear. Backspace works as backspace. / works alright in my browser. The search engine must be a conflicting functionality in the browser you use. I did add short description of few buttons commented on the JS code; i didnt wanna include a legend on the screen.
My formula does not run similar operations from left to right, i know its a flaw. Instead it first runs all multiplications, then divisions, then addition and so on. I did not realize dividing by zero would produce an infinity result. I added a condition in the case there is zero division to return 0 instead.
Thanks for spotting those tiny conditions again

I gotta admit i was a bit careless when naming my variables this project, as i used many, i started to rush names which made mostly sense to me. I placed my in front of few names which sounded too generic and possible to produce conflict like class and button. I did not realize it can cause people decide this marks the code I wrote, that would be embarrassing :wink:

Yes, I as a developer have the ability to read your code and figure this out. Normal users will not. You should include instructions for things like this. Now are they necessary for this particular project? No. But I’m trusting you realize that in a real world project you would need them.

I had only tested this in Firefox. I just tested in Chrome and the sound seems to work fine. If you have the time I would test for yourself in FF and see if you can replicate and if so fix.

Hey @kevinSmith , thanks for thorough reply!
I agree, i did not choose the color template so well. I got carried with a concept i had in mind and eventho it didnt turn out so well, i kept it. I know the sound isnt rly adding much quality to the project, but like you sad, i was having fun and just trying additional functionalities. The audio, i simply pasted the most fitting to my ears sound from the Drum Machine project. I recall an old calculator my parents had, when i was a child, it played sound when you click a button and it had a button to turn the sound on and off, so thats where i took inspiration for it :wink:
The reason i didnt write all the data i need fair and square and instead ‘produce’ it via functions is the shorten the code and also, it makes it easier to manipulate in case i want to make global changes to my data objects/arrays. I agree tho, it might be a bad approach, it certainly has its flaws. Im using arrays to store my objects, like you said, to utilize map and similar array methods. I woudnt use { zero: 0, one: 1} , because those prop names(‘zero’ and ‘one’) i use as id names.
You are right i carried away with the comments. I started outlining the main points and then my OCD took over and i felt the urge to add a note to every next point of the code :slight_smile: , some comments exceeding the length of the text i intended, but its also caused by the fact, i often get lost and hardly comprehend the flow of the code in the example projects, where little description of this and that function could make it clear, instead i have to google every bit of code and then make it sense as a whole.
Another active contributor on those forums already pointed me to hooks but i havent added them yet to my arsenal and in the end, its good to get some practice with the ‘old ways’ before jumping to the contemporary approach. Im already using React, yet i havent even learned the vanilla JS to interacting with the HTML code and it caused me great confusion initially.

I agree with you, for a real world app id definitely take more caution in making things more intuitive, or well presented and would also test compatibility with different browsers ^^

As I remember PEMDAS (at least the way I was taught), M and D were on equal level and A and S were equal, so I was taught to think of it as P-E-MD-AS, and things that are equal are done left to right. But there is a certain vagueness to it so people may interpret it differently. For example, when I put “1/1/*0” into my browser I get 0. But it may depend on implementation.

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.