Skip to content

Conversation

@ahmadehsas
Copy link

@ahmadehsas ahmadehsas commented Oct 28, 2025

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

I have completed all the required tasks.

Questions

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

3 similar comments
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@ahmadehsas ahmadehsas added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 28, 2025
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

1 similar comment
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@cjyuan
Copy link
Contributor

cjyuan commented Nov 11, 2025

  • Title is not in the right format.

  • PR branch is not clean.

Can you address these issues?

@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 Nov 11, 2025
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

1 similar comment
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@ahmadehsas ahmadehsas changed the title West Midlands | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Birminham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Nov 12, 2025
@ahmadehsas ahmadehsas changed the title Birminham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Birmingham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Nov 12, 2025
@ahmadehsas ahmadehsas added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 12, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 20, 2025

You can view all the changed files of this PR branch on https://github.com/CodeYourFuture/Module-Data-Groups/pull/805/files

You copied the Sprint-1 folder into the wrong folder; you haven't yet replaced the modified Sprint-1 folder. Can you make this branch clean?

@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 Nov 20, 2025
@ahmadehsas
Copy link
Author

As I had this issue in my previous module, 'Structuring and testing data' then i followed your instructions:
Download your [main branch] as a ZIP file.

In VSCode, switch to this branch (Sprint-2).

Copy these files from the ZIP file to this branch (make sure they are copied to the right folder)
Push the change to GitHub.

I did this, and now I am a little bit confused, and I need your clarification in more detail.

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

cjyuan commented Nov 20, 2025

You did everything right, except copying the "Sprint-1" folder to the right folder.

I think you can easily fix the issue by

  • Deleting "Sprint-1" folder in the top-level folder in your branch
  • Move "Sprint-2/Sprint-1" to the top-level folder in your branch

If you need interactive assistance, I am available today or tomorrow evening. You can DM me on Slack, "CJ Yuan".

@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 Nov 20, 2025
@ahmadehsas ahmadehsas added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. 📅 Sprint 2 Assigned during Sprint 2 of this module and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 21, 2025
// This means that we need to check if the property belongs to the object using hasownproperty whenever we loop through an object with the `for ... in` loop.

for (const key in author) {
if (author.hasOwnProperty(key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce the if-statement on line 21? Can you justify its need?

Copy link
Author

Choose a reason for hiding this comment

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

When I tested my code without the 'if ... statement', it worked, but the if ... statement is neccessary if there might be properties inherited from the prototype chain.
So the 'if' check doesn't change the output.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • You can output the value of key within the loop to see what values you get
  • You can also use AI to help you clarify some concept

Comment on lines 44 to 54
test("contains on an array returns false", () => {
expect(contains([1, 2, 3], 4)).toBe(false);
});

test("contains on an array of length returns true", () => {
expect(contains([1, 2, 3, 4], "length")).toBe(true);
}); // the test returns true because 'length' is a property of the array object

test("contains given on array as input return false", () => {
expect(contains([1, 2, 3], "a")).toBe(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these function calls return what you expect?

  contains([1, 2, 3], "0");
  contains(null, "a");
  contains(undefined, "a");
  contains(1234, "a");

Copy link
Author

Choose a reason for hiding this comment

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

My function checks for its own property on an object. For the first input `contains([1, 2, 3], "0"); return true because arrays are objects and "0" is a property key.
Inputs like 'null', 'undefined', and '123' will cause errors because they are not objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case you may want to update the test descriptions on lines 44 and 52.

Comment on lines 6 to 11
const keyValuePairs = queryString.split("&");

for (const pair of keyValuePairs) {
const [key, value] = pair.split("=");
const [key, value] = pair.split(/=(.+)/); // We use regex to split only at first.
queryParams[key] = value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function call return what you expect?

  parseQuerySring("A=");  

In real query string, both key and value are percent-encoded or URL encoded.
For example,

tags%5B%5D=hello%20world -> key is tags[], value is hello world

Can your function handle URL-encoded query string?

Suggestion: Look up "How to decode a URL-encoded string in JavaScript".

throw new Error("Input must be an array");
// check if the input ia an array. if not, throw an error.
}
const tally = {};
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.

Comment on lines 23 to 26
// The current return value is {1: "a"}

// b) What is the current return value when invert is called with { a: 1, b: 2 }
// The current return value is {1: "a", 2: "b"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The "current return value" refers to the value returned by the original (unmodified) function.

With the original function, the objects you described on line 23 and 26 are not the objects returned by the function.

Can you find out the actual object returned by the original function?

@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 Nov 21, 2025
@ahmadehsas ahmadehsas added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 23, 2025
Comment on lines 20 to 24
for (const key in author) {
if (author.hasOwnProperty(key)) {
// if (author.hasOwnProperty(key)) {
console.log(`${key}: ${author[key]}`);
}
}
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving commented out unused code can make code review difficult. Can you keep the the code clean by removing all unnecessary code?
image

Comment on lines 48 to 56
test("contains on an array of length returns true", () => {
expect(contains([1, 2, 3, 4], "length")).toBe(true);
// expect(contains([1, 2, 3, 4], "length")).toBe(true);
expect(contains(null, "a", "length")).toBe(true);
}); // the test returns true because 'length' is a property of the array object

test("contains given on array as input return false", () => {
expect(contains([1, 2, 3], "a")).toBe(false);
// expect(contains([1, 2, 3], "a")).toBe(false);
expect(contains(1234, "a")).toBe(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect the function to accept an array as its first parameter or not? These two test descriptions are contradicting each other.

Comment on lines 23 to 26
// The current return value is {'1': 'a'}

// b) What is the current return value when invert is called with { a: 1, b: 2 }
// The current return value is {1: "a", 2: "b"}
// The current return value is {'1': 'a', '2': 'b'}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are still not the objects returned by the original (unmodified) function.

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

Labels

Module-Data-Groups The name of the module. Reviewed Volunteer to add when completing a review with trainee action still to take. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants