Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java script week3 #1

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Java script week3 #1

wants to merge 9 commits into from

Conversation

Veronika121
Copy link
Owner

No description provided.

Copy link

@yash-kapila yash-kapila left a comment

Choose a reason for hiding this comment

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

Hi Veronika - I have finished reviewing your homework and it looks good. However, I have left a couple of comments and would be good if you could go through them and incorporate the changes. Good work and see you tomorrow.

];
const index = Math.floor(Math.random() * 10);
const compliment = compliments[index];
return `You are ${compliment}, ${name}!`;

Choose a reason for hiding this comment

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

Very nicely done and good use of variables.

// TODO complete this function
age = age * 7;

Choose a reason for hiding this comment

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

I think you can remove the TODO comments once you have implemented the logic. Also, in this case, I would recommend to create a new variable here(if needed) instead of mutating the function argument. It would be much cleaner. For example,

function calculateDogAge(age) {
  const dogAge = age * 7;
  return `Your doggie is ${dogAge} years old in dog years!`;
}

or

function calculateDogAge(age) {
  return `Your doggie is ${age * 7} years old in dog years!`;
}

const location = selectRandomly(address);
const partnerName = selectRandomly(name);

return `You will be a ${jobTitle} in ${location}, married to ${partnerName} with ${numKids} kids.`;

Choose a reason for hiding this comment

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

👍

shoppingCart.push(groceryItem);
}
const groceriesString = shoppingCart.join(', ');
return `You bought ${groceriesString}!`;

Choose a reason for hiding this comment

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

👍

Comment on lines 18 to 23
if (newArr.length < 3) {
newArr.push(groceryItem);
} else {
newArr.shift();
newArr.push(groceryItem);
}

Choose a reason for hiding this comment

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

What if you did this instead? A bit cleaner, right?

  const newArr = [...arr];
  newArr.push(groceryItem);

  if (newArr.length > 3) {
    newArr.shift();
  }
  return newArr;

};

function calculateTotalPrice(/* TODO parameter(s) go here */) {
// TODO replace this comment with your code
function calculateTotalPrice(cart) {

Choose a reason for hiding this comment

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

Hi Veronica - although this logic works fine but you could improve it slightly. Firstly, you should try to avoid using for-in loops for iterating over objects. Try to use Object.keys() instead. The difference between the two can be found out on google, it's something to do regarding iterating over parent object properties. Secondly, if you use Object.keys, you could also look at using the array's reduce method. Something like below:

function calculateTotalPrice(cart) {
  const total = Object.keys(cart).reduce((acc, elem) => acc + cart[elem], 0);
  return `Total: ${total}`;
}

for (const employee of arrEmployees) {
const { name, occupation, email } = employee;
const employeeNew = { name, occupation, email };
newArrEmployees.push(employeeNew);

Choose a reason for hiding this comment

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

🆗

@Veronika121
Copy link
Owner Author

Hello Yash! Thank you for your comments! I hope I fixed everything.

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