Skip to content

final update - Asher Pe'er#22

Open
AsherPe wants to merge 1 commit into
ColmanDevClubORG:mainfrom
AsherPe:feature/first-update
Open

final update - Asher Pe'er#22
AsherPe wants to merge 1 commit into
ColmanDevClubORG:mainfrom
AsherPe:feature/first-update

Conversation

@AsherPe

@AsherPe AsherPe commented Dec 6, 2025

Copy link
Copy Markdown

No description provided.

@AsherPe AsherPe changed the title final update final update - Asher Pe'er Dec 6, 2025

@Tamir198 Tamir198 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey left you some comments

Comment thread script.js

number.forEach(button => {
button.addEventListener('click', function () {
console.log(button.textContent)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the console

Comment thread script.js
console.log(button.textContent)
input = input + button.textContent
if (operation !== "")
display.textContent = x + " " + operation + " " + input

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add return statement in here then you will not need the else

Comment thread script.js



const op = [add, sub, mul, div]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe give a more meaningful name? like operators

Comment thread script.js

y = input
num1 = Number(x)
num2 = Number(y)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a NAN check in here

Comment thread script.js
break
case "/":
if (num2 == 0) {
sum = "Cannot divide by zero" // exception

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think that you need the comment, its not adding any thing.
For me the code is understandable as is

Comment thread script.js
}
}

if (sum=="Cannot divide by zero") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also extract this into constant variable

Comment thread script.js
y = ""
sum = ""
operation = ""
display.innerHTML = "  "

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never use innerHTMl its very dangerous method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants