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

NW-6 | Fikret Ellek | JS3| [TECH ED] Array Destructuring | WEEK-3 #311

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

Conversation

fikretellek
Copy link

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.

Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Comment on lines +1 to +10
function isGryffindor(list) {
list.map(({ firstName, lastName, house }) => {
if (house === "Gryffindor") console.log(firstName, lastName, house);
});
}
function havePet(list) {
list.map(({ firstName, lastName, pet, occupation }) => {
if (pet && occupation === "Teacher") console.log(firstName, lastName, pet);
});
}

Choose a reason for hiding this comment

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

Good attempt!

It looks like you are almost there, but be careful when reading requirements, you did not need to log the house in either task, also the first and last name needed to be separated by a space. This code will print HarryPotterGryffindor rather than Harry Potter.

Other improvements:

I would argue however that Array.map() is not the most appropriate method for this task.

Array.map() creates a new instance of an array and returns a value on every iteration, and in this case, you are not assigning the value of this new array to a variable, secondly, you are not returning anything inside the callback function (the function passed into Array.map()).

This means if you assigned the new array to a variable it would just be an array with X number of null values.

In my opinion, there are two improved ways to do this:

First (cleanest):

  list
       .filter( person => person.house === "Gryffindor" )
       .forEach( person => console.log(`${person.firstName} ${person.lastName}`) );

Article on Array.filter() method

Second:

  list.forEach( person => {
     if ( person.house === "Gryffindor" ) 
     {
         console.log(`${person.firstName} ${person.lastName}`)
     }
  } );

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 Jack, using methods one after another is really simple and effective usage. I didn't think about it before.

I see the difference between map and forEach better now. I always mixed those two.

Comment on lines +10 to +20
function printBill(list) {
let total = 0;
console.log("QTY", "\t", "ITEM", "\t\t", "TOTAL");
list.map(({ quantity, itemName, unitPrice }) => {
console.log(quantity, "\t", itemName, "\t", unitPrice * quantity);
total += quantity * unitPrice;
});
console.log("Total:", total);
}

printBill(order);

Choose a reason for hiding this comment

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

Good job!

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.

that's a really useful method. I wish I knew the importance of code review and learn from it long before. Thank you very much for your brilliant and constructive feedback. Sorry for replying after a month. I never knew learning from code review would be this effective. 🙏

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