Skip to content

Westmidlands/Millena-Mesfin/Module Data Groups Sprint#2/Week 10 #485

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

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

Conversation

Millena28
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.

@Millena28 Millena28 added the Needs Review Participant to add when requesting review label Apr 5, 2025
Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Code is very elegant!

I think there are some improvements you can make.

Feel free to mark this PR as "Complete" after you made the changes.

Comment on lines +14 to +18

for (const key in author) {
console.log(key);
}

Copy link

Choose a reason for hiding this comment

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

Can you modify this to output the property values instead of property names?

Comment on lines +1 to +3
function contains(obj,key) {
return (key in obj);
}
Copy link

Choose a reason for hiding this comment

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

Consider the following two approaches for determining if an object contains a property:

  let obj = {}, propertyName = "toString";
  console.log( propertyName in obj );                // true
  console.log( Object.hasOwn(obj, propertyName) );   // false

Which of these approaches suits your needs better?
For more info, you can look up JS "in" operator vs Object.hasOwn.

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 cj, I got to differentiate the (propertyName in obj) suits to my function.

Comment on lines +50 to +54
test("contains an invalid parameter like array should return false" ,() => {
const obj = ["code","your","future"];
expect(contains(obj,'age')).toBe(false);
expect(contains(obj,'address')).toBe(false);
});
Copy link

Choose a reason for hiding this comment

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

An array is a kind of object in JS.
The indexes serve as the keys (or property names) in an array. So contains(["code","your","future"], '0') would return true.
If the function needs to return false when the parameter is an array, the function will need to check if the parameter is an array or not.

Comment on lines +44 to +49
test("An array with invalid input should throw an error", () => {
const count = ["hello", "hey", "hey", "hello", "hello", "hi"];
const expectedOutput = {hello: 3, hey: 2, hi: 1 };

expect(tally(count)).toEqual(expectedOutput);
});
Copy link

Choose a reason for hiding this comment

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

The description and the code does not match.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Apr 8, 2025
@Millena28 Millena28 added the Complete Participant to add when work is complete and review comments have been addressed label Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants