Manchester | 25-ITP-Sep | Mahtem T. Mengstu | Sprint 2 |Sprint-2-courseWork#913
Manchester | 25-ITP-Sep | Mahtem T. Mengstu | Sprint 2 |Sprint-2-courseWork#913Mahtem wants to merge 26 commits intoCodeYourFuture:mainfrom
Conversation
Sprint-2/debug/recipe.js
Outdated
| console.log(`${recipe.title} serves ${recipe.serves} | ||
| ingredients: | ||
| ${recipe}`); | ||
| ${recipe.ingredients.join(", ")}`); |
There was a problem hiding this comment.
Can you make each ingredient to appear on a new line (to meet the specification on line 4)?
There was a problem hiding this comment.
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.
Sprint-2/implement/contains.js
Outdated
| if (typeof obj !== "object" || obj === null || Array.isArray(obj)) { | ||
| return false; | ||
| } | ||
| return key in obj; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) ); // falseWhich 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);
Sprint-2/implement/contains.test.js
Outdated
| // Then it should return false or throw an error | ||
|
|
||
| test("Should return false when invalid parameters are used", () => { | ||
| expect(contains([], "a")).toEqual(false); // array |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Sprint-2/implement/querystring.js
Outdated
|
|
||
| // If '=' not found we have a key exists but value is empty | ||
|
|
||
| paramKey = pair; |
There was a problem hiding this comment.
You missed decode this paramKey.
There was a problem hiding this comment.
You missed decode this
paramKey.
Thank you for pointing out that, I have modified it to be with added decode.
param key = decodeURIComponent(pair);
Sprint-2/implement/tally.js
Outdated
| return {}; | ||
| } | ||
|
|
||
| const counts = {}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
…ients appear on new line.
cjyuan
left a comment
There was a problem hiding this comment.
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. |
Sprint-2/implement/contains.test.js
Outdated
| 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 |
There was a problem hiding this comment.
The test on line 56 does not quite match the test description.
There was a problem hiding this comment.
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
|
All good. Well done! |
Thank you. |
Learners, PR Template
Self checklist
Changelist
In the data-groups module Sprint -2 exercises. functions implemented and tests passed.
Questions
Questions and review requests will be done in slack.