Feedback

[DanForder]

Site

• Great looking design with clear colours, and you’ve got all the basic functionality down here
• Couple of little bugs, for example I see “NaN” appear when I press an operator twice, and if I press “=” at the start, the calculator breaks. These are pretty minor, but users will always break your app if they can! So remember to test edge cases.
• Remember to include a README.md file that gives a bit of insight into the project and gives a link to the site etc. Take a look at this previous student’s for some inspiration if you’d like: https://github.com/alessandrothedev/calculator

Code

• You haven’t used BEM conventions here, which was part of the requirements, definitely use that block__element--modifier class naming for your game! Really need to get you comfortable with writing HTML using BEM before the client project and you’re all working on the same codebase.
• Your JS code has a lot of functionality, but your code is still nice and concise which is awesome to see. I’d like to see you clean up your addEventListener logic a bit however, which is going to make it a lot easier to modify you code going forward

// your code
decimalButton.addEventListener("click", (e) => {
  decimalString = e.target.value;

  if(!currentString.includes('.')) {
    currentString += '.'
  } else{
    return currentString;
   }

});

// refactor this to reference a separate function
const handleDecimalPress = (e) => {
  decimalString = e.target.value;
  if (!currentString.includes('.')) {
    currentString += '.'
  } else {
    return currentString;
  }
}

decimalButton.addEventListener("click", handleDecimalPress);

• Now that decimal press logic has been abstracted into it’s own function, we can look at refactoring it a bit! Remember functions should only do one thing, in this case we add a decimal place to the current string if we’re allowed. So we actually don’t need that else or return inside it:

// remove decimal string, it's already "."
// remove unused "e" parameter
// remove else statement
const handleDecimalPress = () => {
  if (!currentString.includes('.')) {
    currentString += '.'
  }
}

decimalButton.addEventListener("click", handleDecimalPress);

Overall this is a great result Ahmed! For your game, I’d like to see you work on pulling out your logic into multiple functions. This is going to make it a lot easier for you/ others to follow your code without needing to read it line by line.

[ahmedhomar]

Thanks Dan. I was hoping to implement BEM but I ran out of time. I'll make sure to use it in the game project. Your feedback cleaning up my code is very helpful: thanks for the great examples.