Skip to content

Conversation

@Mahtem
Copy link

@Mahtem Mahtem commented Dec 6, 2025

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

In the data-groups module Sprint -2 exercises. functions implemented and tests passed.

Questions

Questions and review requests will be done in slack.

@Mahtem Mahtem added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 2 Assigned during Sprint 2 of this module labels Dec 6, 2025
console.log(`${recipe.title} serves ${recipe.serves}
ingredients:
${recipe}`);
${recipe.ingredients.join(", ")}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make each ingredient to appear on a new line (to meet the specification on line 4)?

Copy link
Author

Choose a reason for hiding this comment

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

Can you make each ingredient to appear on a new line (to meet the specification on line 4)?

Thank you @cjyuan I have modified "${recipe.ingredients.join("\n")}`) to make ingredients appear on new line.

if (typeof obj !== "object" || obj === null || Array.isArray(obj)) {
return false;
}
return key in obj;
Copy link
Contributor

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.

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.

Thank you, I have come to know that, console.log( propertyName in obj ); inherits prototype properties,
So, I have modified return key in obj; to return Object.hasOwn(obj, key);

// Then it should return false or throw an error

test("Should return false when invalid parameters are used", () => {
expect(contains([], "a")).toEqual(false); // array
Copy link
Contributor

Choose a reason for hiding this comment

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

Arrays are objects in JavaScript, and they do have property names -- just not the same ones as objects.
Which keys do arrays have, and how does that affect how reliable your test is?

When testing whether the function handles arrays properly, try using a key that an array might
realistically contain
. Otherwise, you might get a passing test even if the function isn't checking for arrays at all.

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 @cjyuan, I have tried to include test which address that an array might realistically contain. e.g. below
expect(contains([1, 2, 3], "1")).toEqual(true); // "1" is a key
expect(contains([1, 2, 3], "3")).toEqual(false); // "3" is not a key


// If '=' not found we have a key exists but value is empty

paramKey = pair;
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed decode this paramKey.

Copy link
Author

Choose a reason for hiding this comment

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

You missed decode this paramKey.

Thank you for pointing out that, I have modified it to be with added decode.

param key = decodeURIComponent(pair);

return {};
}

const counts = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the following function call returns the value you expect?

  tally(["toString", "toString"]);

Suggestion: Look up an approach to create an empty object with no inherited properties.

Copy link
Author

Choose a reason for hiding this comment

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

Does the following function call returns the value you expect?

  tally(["toString", "toString"]);

Suggestion: Look up an approach to create an empty object with no inherited properties.

Thank you, I tested it and it returns {toString: 'function toString() { [native code] }11'} , as it is getting the inherent property and I have come to know that const counts = Object.create(null); solves the case.

Comment on lines 30 to 56
function countWords(string) {
// validation of input
if (typeof string !== "string") {
throw new Error("Input should be a string");
}
if (string === "") return {};
const counts = {};

// Remove punctuation and lowercase everything

const cleanText = string.replace(/[.,!?]/g, "").toLowerCase();
const words = cleanText.split(" ");

// Count word frequencies
for (const word of words) {
counts[word] = (counts[word] || 0) + 1;
}

// Sort by frequency (most common first)
const sorted = Object.entries(counts).sort((a, b) => b[1] - a[1]);

return Object.fromEntries(sorted);
}

console.log(countWords("Hello!, my friend, You and me, and you."));

// In count-words.js function countWords implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the following function calls return what you expect?

countWords("constructor constructor");
countWords("          Hello   World      ");

Note: Change is optional as this is a stretch exercise.

Copy link
Author

Choose a reason for hiding this comment

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

Do the following function calls return what you expect?

countWords("constructor constructor");
countWords("          Hello   World      ");

Note: Change is optional as this is a stretch exercise.

Thank you @cjyuan, I have modified it to handle inherent property cases and remove empty strings.

const counts = Object.create(null);
const words = cleanText.split(" ").filter(Boolean);

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 8, 2025
@Mahtem Mahtem added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 10, 2025
Copy link
Contributor

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

Changes look great.

Have you run all the Jest test scripts to ensure everything works as expected after making the latest changes?

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 10, 2025
@Mahtem
Copy link
Author

Mahtem commented Dec 10, 2025

Changes look great.

Have you run all the Jest test scripts to ensure everything works as expected after making the latest changes?

Hello @cjyuan , yes all the jest scripts passed.

Comment on lines 55 to 57
test("Should return false when invalid parameters are used", () => {
expect(contains([1, 2, 3], "1")).toEqual(true); // "1" is a key
expect(contains([1, 2, 3], "3")).toEqual(false); // "3" is not a key
Copy link
Contributor

Choose a reason for hiding this comment

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

The test on line 56 does not quite match the test description.

Copy link
Author

Choose a reason for hiding this comment

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

The test on line 56 does not quite match the test description.

Yes, it's right.. I have tried to make is separated as follows:

test("Should correctly detect keys in arrays", () => {
expect(contains([1, 2, 3], "1")).toEqual(true); // "1" is a key
expect(contains([1, 2, 3], "3")).toEqual(false); // "3" is not a key
});

test("Should return false when invalid parameters are used", () => {
expect(contains(null, "a")).toEqual(false); // null
expect(contains(5, "a")).toEqual(false); // number
expect(contains("hello", "a")).toEqual(false); // string

@cjyuan
Copy link
Contributor

cjyuan commented Dec 10, 2025

All good. Well done!

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Dec 10, 2025
@Mahtem
Copy link
Author

Mahtem commented Dec 10, 2025

All good. Well done!

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants