Skip to content

finished calculator project#13

Open
ShaharAk1 wants to merge 2 commits into
ColmanDevClubORG:mainfrom
ShaharAk1:main
Open

finished calculator project#13
ShaharAk1 wants to merge 2 commits into
ColmanDevClubORG:mainfrom
ShaharAk1:main

Conversation

@ShaharAk1

Copy link
Copy Markdown

No description provided.

@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 index.html
<button class="number" id="number-7">7</button>
<button class="number" id="number-8">8</button>
<button class="number" id="number-9">9</button>
<button class="number" data-value="7" id="number-7">7</button>

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.

Why do you need this property ?

Comment thread index.html
<button id="subtract">-</button>
<button id="multiply">*</button>
<button id="divide">/</button> -->
<button data-value="+" id="add">+</button>

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.

Same question in here, why are you using it?

Comment thread script.js
@@ -0,0 +1,66 @@
const display = document.querySelector(".display");
let reset = true;

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.

reset sounds like a function, maybe call this isReseted or something that will indicate the current state

Comment thread script.js
let reset = true;

const calculate = () => {
let result = 0;

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.

Result could be global variable (global for this file) so you will not create it every time again and again

Comment thread script.js
display.textContent = result;
}
if (fulleq.includes("-")){
let minieq = fulleq.split("-");

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 have 4 pieces of the same code, maybe extract it to function? the only different thing between them is the operator

Comment thread script.js
}
if (fulleq.includes("-")){
let minieq = fulleq.split("-");
result = Number(minieq[0]);

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.

This could ne NAN, better add a check for it

Comment thread script.js
if (reset === true) {
display.textContent = btn.dataset.value;
reset = false;
}

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.

If you will add return in here you will not need the else

Comment thread script.js

document.getElementById("clear").addEventListener("click", () => {
reset = true;
display.textContent = "0";

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.

Save your hardcoded strings into constant variables

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.

3 participants