Skip to content

London | 26-ITP-Jan | Damian Dunkley | Sprint 2 | Data-Groups#993

Closed
DamianDL wants to merge 23 commits intoCodeYourFuture:mainfrom
DamianDL:sprint-2
Closed

London | 26-ITP-Jan | Damian Dunkley | Sprint 2 | Data-Groups#993
DamianDL wants to merge 23 commits intoCodeYourFuture:mainfrom
DamianDL:sprint-2

Conversation

@DamianDL
Copy link
Copy Markdown

@DamianDL DamianDL commented Mar 11, 2026

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

All Sprint 2 exercises updated and code changed

Questions

Please review. If Invert is not correct- changing the function to make it work and putting in naive consol.log tests, please provide some details of what is required? Thanks

@github-actions

This comment has been minimized.

4 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DamianDL DamianDL added 📅 Sprint 2 Assigned during Sprint 2 of this module Module-Data-Groups The name of the module. labels Mar 11, 2026
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@DamianDL DamianDL added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 12, 2026
@github-actions

This comment has been minimized.

@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 12, 2026
@DamianDL DamianDL added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 12, 2026
Copy link
Copy Markdown
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.

You did a good job in rejecting invalid parameters in all functions.
I only have some suggestions.

Comment thread Sprint-2/debug/recipe.js
Comment thread Sprint-2/implement/contains.js Outdated
!!obj && // confirm that obj is not null or undefined. If undefined or null, return false.
typeof obj === "object" && // confirm that obj is an object. If not an object, return false.
!Array.isArray(obj) && // confirm that obj is not an array. If an array, return false.
Object.prototype.hasOwnProperty.call(obj, prop)// check if the object has the property. If it does, return true; otherwise, return false.
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.

Could also use Object.hasOwn().

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.

Removed the for loop, replaced with a console.log... .join construction.

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.

Removed the for loop, replaced with a console.log... .join construction.

Changed Object.prototype.hasOwnProperty() with just Object.hasOwnProperty() to simplify code

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.

I meant, we can also use Object.hasOwn(obj, prop).

// Then it should return false or throw an error
test("contains returns false for invalid input types", () => {
const arr = [1, 2, 3];
expect(contains(arr, "0")).toBe(false); // Arrays do not have properties like objects, so it should return false
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.

Note: An array is a kind of objects and its indexes are its keys (properties).

So for an implementation that does not check if the given value is an array using Array.isArray(), contains([1, 2, 3], "0") could have returned true.

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.

I think that you're saying that my use of !Array.isArray(obj) as a check only works because I have included the property and if I had !Array.isArray(), this would allow 0 to be found in an array of 1, 2, 3 because 1 has a property of "0". Have I understood that correctly or are you expecting me to make changes? Thanks

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.

Sorry that my comment wasn't clear enough.

I meant say the description given in the comment is not quite correct.

// Arrays do not have properties like objects, so it should return false

The function should return false because the first argument is an array (and not because it does not have properties).

Comment thread Sprint-2/implement/lookup.js Outdated
Comment thread Sprint-2/implement/querystring.js
Comment thread Sprint-2/implement/tally.js Outdated
@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 20, 2026
@DamianDL DamianDL 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 Mar 24, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Mar 24, 2026

Changes look good. Well done.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 24, 2026
@illicitonion
Copy link
Copy Markdown
Member

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

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. Module-Data-Groups The name of the module. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants