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.
