Skip to content
This repository was archived by the owner on Oct 26, 2020. It is now read-only.

First pull request #22

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Osman-Hajr
Copy link

Commiting changes for the homework, exercise and some of the extras.

Your Details

Your Name: Osman
Your City: Birmingham
Your Slack Name: Osman Hajr

Homework Details

Module: JavaScript-Core-1.
Week: 1.

Osman-Hajr and others added 10 commits June 19, 2020 16:33
Commiting changes for the homework, exercise and some of the extras.
Commiting my answers to the exercises
comitting answers to exercises
more commit to exercise and mandatory
@EmilePW EmilePW self-requested a review July 13, 2020 19:13
Copy link

@EmilePW EmilePW left a comment

Choose a reason for hiding this comment

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

Hi @Osman-Hajr, this is really great work, I think you have a good understanding of the material here. The only note I had was for your calculateSalesTax function below:

Careful not to have redundant code, e.g. for calculating sales tax you have used the priceUnit parameter twice and multiplied it by 1 which is unnecessary:

function calculateSalesTax(priceUnit) {
	let taxedPrice = priceUnit + (priceUnit * 1) / 5;
	return taxedPrice;
}

Instead, you could write it like:

function calculateSalesTax(priceUnit) {
	let taxedPrice = priceUnit * (1 + 1 / 5);
	return taxedPrice;
}

Which will do the same thing with less repetition.

Like I said, this is great work overall!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants