Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

Conversation

Afsha10
Copy link

@Afsha10 Afsha10 commented Jun 2, 2023

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@@ -4,7 +4,8 @@ const personOne = {
favouriteFood: "Spinach",
};

function introduceYourself(___________________________) {

function introduceYourself({ name, age, favouriteFood } = personOne) {

Choose a reason for hiding this comment

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

Do you need to set a default for the object that is passed in?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing it out, Jack! Just refactored it.

Comment on lines 76 to 90
let gryffindorStudentNames = hogwarts.filter(({house}) => {
return house === "Gryffindor";
});
for (const student of gryffindorStudentNames) {
let { firstName, lastName } = student;
console.log(firstName, lastName);
}

let teachersWithPets = hogwarts.filter(({occupation, pet}) => {
return (occupation === "Teacher") && (pet !== null);
});
for (const teacher of teachersWithPets) {
let { firstName, lastName } = teacher;
console.log(firstName, lastName);
}

Choose a reason for hiding this comment

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

Well done finding a valid solution for both problems, a few notes to improve these solutions.

You are using the filter method and assigning the results to a new variable. While this is an acceptable approach, you have used the let declaration when this variable is not reassigned at any stage, so const would have been the correct declaration to use.

Also, you are using two loops for each solution, when it can be simplified to use 1. This can be achieved using the Array.forEach method or just a for loop which you already have. Then using a condition to check the object values before logging the character name.

Copy link
Author

Choose a reason for hiding this comment

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

Hey Jack!
Thank you so much for your valuable feedback!
I will make the changes now.

Copy link
Author

Choose a reason for hiding this comment

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

I made the changes according to you second suggestion.

Comment on lines 11 to 21
console.log("QTY ITEM TOTAL");
let totalPrice = 0;
order.forEach(({quantity, itemName, unitPrice}) => {
const itemPrice = `${unitPrice * quantity}`;
console.log(`${quantity}\t${itemName}\t\t\t\t${itemPrice}`);
totalPrice = totalPrice + unitPrice * quantity;
})
console.log(`Total: ${totalPrice}`);


// Question: Why doesn't it work when I do totalPrice = totalPrice + itemPrice in line 16
Copy link

@JackRogers9 JackRogers9 Jun 8, 2023

Choose a reason for hiding this comment

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

To answer your question about why doing totalPrice = totalPrice + itemPrice; does not work, it is because on line 14 you are defining itemPrice as a string by using template literals.

If you were to remove the template literals and do const itemPrice = unitPrice * quantity;, then it would work.

Choose a reason for hiding this comment

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

Also, an improvement you could make to this solution would be to use Array.reduce() to create a running aggregate instead of defining totalPrice outside of the loop.

Here is a link that explains using reduce to calculate a sum.

https://linuxhint.com/call-reduce-on-an-array-of-objects-to-sum-their-properties/

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much for noticing my question and answering it! Thank you for the helpful article! I have made some changes.

Afsha10 added 5 commits June 8, 2023 21:19
Refactored the code by using the for of loop I had and using a condition to check the object values before logging the fantasy characters' names
Cleared some confusion and refactored a bit
…e to a variable called totalPrice and putting the variable to the console.

Refactored after some great suggestions made by the reviewer. Refactored by using .reduce method to create a running aggregate instead of defining totalPrice outside of the loop, and by assigning the returned value to a variable called totalPrice and putting the variable to the console.
@Afsha10 Afsha10 closed this Jun 9, 2023
@Afsha10 Afsha10 deleted the array-destructuring-project branch June 9, 2023 15:48
@Afsha10 Afsha10 restored the array-destructuring-project branch June 9, 2023 16:07
@Afsha10 Afsha10 reopened this Jun 9, 2023
@Afsha10 Afsha10 closed this Jun 9, 2023
@Afsha10 Afsha10 deleted the array-destructuring-project branch June 9, 2023 16:28
@Afsha10 Afsha10 restored the array-destructuring-project branch June 9, 2023 16:31
@Afsha10 Afsha10 reopened this Jun 9, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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