Skip to content

London | 26-ITP-May | Zadri Abdule | Sprint 2 | Exercises - #1302

Open
Zadri415 wants to merge 12 commits into
CodeYourFuture:mainfrom
Zadri415:sprint-2-only
Open

London | 26-ITP-May | Zadri Abdule | Sprint 2 | Exercises#1302
Zadri415 wants to merge 12 commits into
CodeYourFuture:mainfrom
Zadri415:sprint-2-only

Conversation

@Zadri415

@Zadri415 Zadri415 commented Jul 26, 2026

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

  • address.js — debug fixes
  • author.js — debug fixes
  • recipe.js — debug fixes
  • contains.js — implement contains
  • contains.test.js — tests for contains
  • lookup.js — implement lookup
  • lookup.test.js — tests for lookup
  • querystring.js — robust querystring parser (handles +, %, duplicate keys, values with '=')
  • tally.js — implement tally (frequency counter)
  • tally.test.js — tests for tally
  • invert.js — invert implementation (collects duplicate-value keys into arrays)
  • invert.test.js — tests for invert (including duplicate-values)
  • count-words.js — countWords implementation returning frequency map
  • mode.js — refactored calculateMode into helpers
  • till.js — fixed totalTill implementation (parses coin labels) and exported

@Zadri415 Zadri415 added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. labels Jul 26, 2026
Comment thread Sprint-2/implement/contains.test.js Outdated
// When passed to contains
// Then it should return false or throw an error
test("contains with invalid parameters returns false", () => {
expect(contains([], 'a')).toBe(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.

contains([], 'a') could also return false simply because "a" is not a key of the array.

In JS, arrays are also objects, with their indices acting as keys. A proper test should use a non-empty array along with a valid key of the array to ensure the function returns false specifically because the input is an array, not because the key is missing.

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. Made the suggested changes.

Comment thread Sprint-2/implement/querystring.js Outdated
Comment thread Sprint-2/implement/tally.js Outdated
Comment thread Sprint-2/interpret/invert.js Outdated
Comment on lines +32 to +35
// {"1":"a"}

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

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.

The "current return value" in questions (a), (b), and (d) refer to the return value of the original faulty function.

Comment thread Sprint-2/interpret/invert.js Outdated
}
} else {
inverted[v] = key;
}

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: This rewrite version is doing more than just swapping the keys and values. Change is optional.

Supposedly you could change one line of code to fix the problem.

@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 Aug 1, 2026
@cjyuan

cjyuan commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

The Changelist content is not quite accurate.

@Zadri415 Zadri415 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2026
Comment thread Sprint-2/implement/contains.test.js
Comment thread Sprint-2/implement/tally.js Outdated

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 file basically remain unchanged since my previous review. The comments I left on this file still hold.

The most recent commit (30719f0) that says "Update invert.js" only removed an empty line from the file:

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2026
@Zadri415 Zadri415 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 15, 2026

@cjyuan cjyuan left a comment

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.

Looks good.

@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. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 16, 2026
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.

2 participants