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

added task solution #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added task solution #1

wants to merge 1 commit into from

Conversation

mbruhler
Copy link
Owner

Hi,
Great work Artem, I can see that you're getting better in JS. Keep it up!

There are some issues with your code that I'd like to discuss together. The detailed explanation is made in comments but here I also want to add that you should use mocks while testing.

It's always a great idea to create __mocks__ folder and create mock data there, then use it as constants in test cases.

For JS part, I can see that you have troubles with using reduce. I supplied you with some examples but you should spend some time on developing your skills using this function. Iterating arrays is very important.

// avoid using loop and forEach
// replace `if ()` statement with &&, || or ?:
// without nesting
const men = century
Copy link
Owner Author

Choose a reason for hiding this comment

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

Long and hard to read statement. In my opinion it's always a great idea to seperate such logic into function.
Consider separating filtering method into function like this:

function isMenInCentury(p, century) {
    return p.sex === 'm' && Math.ceil(p.died / 100) === century;
}

And then you can use it like this:

const men = century
   ? people.filter((p) => isMenInCentury(p, century))
   : people.filter((p) => p.sex === 'm');


let sum = 0;

men.forEach((m) => sum += m.died - m.born);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Use reduce rather then iterating and assigning to sum. This way you can also inline variable and lower the number of lines in your code.

Consider following code:

return men.reduce((acc, curr) => {
        acc += curr.died - curr.born;
        return acc;
    }, 0) / men.length;


men.forEach((m) => sum += m.died - m.born);

return sum/men.length;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Full example code:

function isMenInCentury(p, century) {
    return p.sex === 'm' && Math.ceil(p.died / 100) === century;
}
function calculateMenAverageAge(people, century) {
    const men = century
        ? people.filter((p) => isMenInCentury(p, century))
        : people.filter((p) => p.sex === 'm');
    
    return men.reduce((acc, curr) => {
        acc += curr.died - curr.born;
        return acc;
    }, 0) / men.length;

}

@@ -37,7 +42,16 @@ function calculateMenAverageAge(people, century) {
* @return {number}
*/
function calculateWomenAverageAge(people, withChildren) {
// write code here
const women = withChildren
? people.filter((p) => p.sex === 'f'
Copy link
Owner Author

Choose a reason for hiding this comment

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

Code is hard to read. Consider creating functions that implement filtering logic like so:

function isWomen(p) {
    return  p.sex === 'f';
}
function hasChildren(p, people){
    return !!people.find((p1) => p1.mother === p.name)
}

Usage:

const women = withChildren
        ? people.filter((p) => womenWithChildren(p, people))
        : people.filter((p) => isWomen(p));


let sum = 0;

women.forEach((w) => sum += w.died - w.born);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Use reduce instead and return the sum instead of assigning to variable and using forEach

return women.reduce((acc, curr) => {
    acc += curr.died - curr.born;
    return acc;
}, 0) / women.length;

@@ -55,7 +69,19 @@ function calculateWomenAverageAge(people, withChildren) {
* @return {number}
*/
function calculateAverageAgeDiff(people, onlyWithSon) {
// write code here
const children = onlyWithSon
Copy link
Owner Author

Choose a reason for hiding this comment

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

Same as above. Separate filtering logic from code using utility functions.


let sum = 0;

children.forEach((ch) => sum += ch.born - people.find(
Copy link
Owner Author

Choose a reason for hiding this comment

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

Use reduce like this

return children.reduce((acc, curr) => {
    acc += curr.born - (people.find((m) => m.name === curr.mother)?.born || 0);
    return acc;
}, 0) / children.length;

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