Skip to content
This repository was archived by the owner on Jan 14, 2024. It is now read-only.

LONDON_10 | SAIM KORKMAZ | JS-1-1#510

Open
nsaimk wants to merge 4 commits into
CodeYourFuture:masterfrom
nsaimk:master
Open

LONDON_10 | SAIM KORKMAZ | JS-1-1#510
nsaimk wants to merge 4 commits into
CodeYourFuture:masterfrom
nsaimk:master

Conversation

@nsaimk
Copy link
Copy Markdown

@nsaimk nsaimk commented Feb 21, 2023

Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in HOW_TO_MARK.md in the root of this repository

Your Details

  • Your Name:
  • Your City:
  • Your Slack Name:

Homework Details

  • Module:
  • Week:

Notes

  • What did you find easy?

  • What did you find hard?

  • What do you still not understand?

  • Any other notes?

Comment on lines +22 to +25
function convertToBRL(price) {
price = price * 5.7 * 0.99;
return parseFloat(price.toFixed(2));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please look into pure functions and argument mutation.

It's considered a bad practice for a function to have side effects. Can you think of which side effect this function has and why would it be bad practice?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

only thing came my mind is "5.7" and "0.99" must have been const

Comment thread extra/2-piping.js Outdated
Comment on lines +37 to +39
let step1 = add(startingValue, 10);
let step2 = multiply(step1, 2);
let goodCode = format(step2);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should try to use const if we know the value is not going to be re-assinged/changed.

Comment thread extra/3-magic-8-ball.js
Comment on lines +89 to +115
if (
answer == "It is certain." ||
answer == "It is decidedly so." ||
answer == "Without a doubt." ||
answer == "Yes - definitely." ||
answer == "You may rely on it."
) {
return "very positive";
} else if (
answer == "As I see it, yes." ||
answer == "Most likely." ||
answer == "Outlook good." ||
answer == "Yes." ||
answer == "Signs point to yes."
) {
return "positive";
} else if (
answer == "Reply hazy, try again." ||
answer == "Ask again later." ||
answer == "Better not tell you now." ||
answer == "Cannot predict now." ||
answer == "Concentrate and ask again."
) {
return "negative";
} else {
return "very negative";
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is fine, but you are using a lot of magic words here. Try to look into what magic words are and how this could be improved.

Hint:
You can either use a JS object or array index to determine whether it's positive or negative

}
function getTotal(a, b) {
total = a ++ b;
total = a + b;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is missing a declaration, could you explain why it worked without a const or let declaration and why it would be a bad practice, even though it works?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i made it global variable by mistake, but has a side effect it is it may cause unexpected behaviour

function concatenate(firstWord, secondWord, thirdWord) {
// Write the body of this function to concatenate three words together.
// Look at the test case below to understand what this function is expected to return.
return firstWord.concat(" ", secondWord.concat(" ", thirdWord));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this works, but could you think of a solution using Template Literals (I think it will be a cleaner solution)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

is it: return ${firstWord}${secondWord}${thirdWord}

Comment thread mandatory/4-tax.js
Comment on lines +8 to +11
function calculateSalesTax(price) {
price = price + price / 5;
return price;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See previous comment about pure functions

Comment thread mandatory/4-tax.js
Comment on lines +23 to +26
function addTaxAndFormatCurrency(price) {
price = price + price / 5;
return `£${price.toFixed(2)}`;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another one on pure functions

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just found out what it is
const newPrice = price + price/5

...a pure function should not modify its input parameters or any other external variables or state

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes! that would be a better approach. We should try avoiding modification of parameters because if you call the same function multiple times you will keep getting different answers.

Copy link
Copy Markdown

@berkeli berkeli left a comment

Choose a reason for hiding this comment

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

Good job on the homework.

I have left a few comments to explore and think about, feel free to reach out on Slack or reply to comments if you would like to discuss them.

Oh and please try to get in the habit of filling out the pull request template, as this is very common in workplace.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants