Skip to content

Sheffield | 26-Jan-ITP | Martha Ogunbiyi| Sprint 2| Coursework/sprint 2#1133

Open
marthak1 wants to merge 9 commits intoCodeYourFuture:mainfrom
marthak1:coursework/sprint-2
Open

Sheffield | 26-Jan-ITP | Martha Ogunbiyi| Sprint 2| Coursework/sprint 2#1133
marthak1 wants to merge 9 commits intoCodeYourFuture:mainfrom
marthak1:coursework/sprint-2

Conversation

@marthak1
Copy link
Copy Markdown

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

Sprint 2 coursework done

@marthak1 marthak1 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 28, 2026
Comment on lines +14 to 16
for (const value in author) {
console.log(value);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this code output the property values such as "Zadie", "Smith", ...?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, i have update to output the values rather the keys

if (obj.constructor !== Object) {
throw new Error("Invalid Parameter");
}
return Object.keys(obj).includes(targetKey);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can also use Object.hasOwn() -- probably a more efficient approach.

Comment on lines 53 to +61
// Given invalid parameters like an array
// When passed to contains
// Then it should return false or throw an error
test("given an invalid parameters, it should return false or throw an error", () => {
const input = ['a', 'b', 'c'];
const target = 'a'
expect(() => contains(input, target)).toThrow();

});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test does not yet confirm that the function correctly returns false when the first argument is an array.
This is because contains(['a', 'b', 'c'], "a") could also return false simply because "a" is not a key of the array.

Arrays are objects, with their indices acting as keys. A proper test should use an array along with a valid key to ensure the function returns false specifically because the input is an array, not because the key is missing.

Comment on lines +2 to +4
if (obj.constructor !== Object) {
throw new Error("Invalid Parameter");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For each of the following function calls, does your function return the value you expect?

const obj = new Map();  // Map object is an object 
obj.foo = 1;
contains(obj, "foo"); 

// According to the spec in the test file, the function should also reject 
// array (even though array is a kind of objects in JS)
contains([0, 0, 0], "1"); 

contains(null, "1");
contains(undefined, "1");

Comment on lines 8 to 18
for (const pair of keyValuePairs) {
const [key, value] = pair.split("=");
const index = pair.indexOf("=");

if (index === -1) {
queryParams[pair] = "";
continue;
}
const key = pair.slice(0, index);
const value = pair.slice(index + 1);
queryParams[key] = value;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the following function call, does your function return the value you expect?

parseQueryString("key1=value1&&key2=value2")

function tally() {}
function tally(list) {
const uniqueItems = {};
if(list.constructor === String){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why only treat string as invalid argument? What about other data types: null, undefined, 123, true, etc.

Comment on lines +6 to +8
for (const frequency of list){
uniqueItems[frequency] = (uniqueItems[frequency] || 0) + 1;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indentation is off.

Have you installed the prettier VSCode extension and enabled "Format on save/paste" on VSCode,
as recommended in https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/blob/main/readme.md ?

@@ -1,3 +1,14 @@
function tally() {}
function tally(list) {
const uniqueItems = {};
Copy link
Copy Markdown
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.

@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 Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants