Skip to content

London | ITP-May-2025 | Seddiq Azam | Module-Data-Groups | Sprint 2 #578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

sedazam
Copy link

@sedazam sedazam commented Jul 10, 2025

You must title your PR like this:

REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME

For example,

London | May-2025 | Carol Owen | Quote-generator

Complete the task list below this message.
If your PR is rejected, check the task list.

-->

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

This pull request includes debugging fixes, implementation improvements, and testing updates across multiple files in Sprint-2 and Sprint-3. Key changes include resolving bugs in object property handling, enhancing functionality for utility functions, and adding comprehensive test cases to ensure correctness. Below is a categorized summary of the most important changes:

Debugging Fixes:

  • address.js: Fixed a bug where address.houseNumber was incorrectly accessed as address[0]. Updated the code to properly define the address object and access its properties. [1] [2]
  • author.js: Corrected the iteration over object properties by replacing for...of with Object.values() to handle non-iterable objects. [1] [2]
  • recipe.js: Adjusted the logging of recipe ingredients by iterating over the array and joining them with newlines for proper formatting. [1] [2]

Implementation Enhancements:

  • contains.js: Implemented the contains function to check for object properties with safety checks for invalid inputs.
  • lookup.js: Added the createLookup function to generate a key-value mapping from an array of pairs.
  • querystring.js: Fixed edge cases in parseQueryString, such as handling keys without values, and updated the test suite. [1] [2]
  • tally.js: Implemented the tally function to count occurrences of items in an array, with error handling for invalid inputs. [1] [2]
  • invert.js: Corrected the invert function to dynamically swap object keys and values and added explanatory comments.

Testing Improvements:

  • contains.test.js: Added comprehensive tests for the contains function, covering valid and invalid inputs, empty objects, and edge cases.
  • lookup.test.js: Added a test for createLookup to validate the output structure and correctness.
  • tally.test.js: Expanded test coverage for the tally function, including cases for empty arrays, duplicate items, and invalid inputs.

Stretch Goals:

  • count-words.js: Implemented a function to count word occurrences in a string, including handling empty or whitespace-only strings.
  • mode.js: Refactored calculateMode by splitting it into smaller functions for frequency calculation and finding the most frequent value. [1] [2]
  • till.js: Fixed and added an alternative implementation for totalTill to correctly calculate total amounts in pounds, with detailed explanations and tests. [1] [2]

Questions

Ask any questions you have for your reviewer.

sedazam and others added 22 commits June 28, 2025 15:34
created	new file called mean.test.js:   prep/mean.test.js
created new file called mean.js
	deleted:    prep/mean.test.js
	modified:   Sprint-2/implement/contains.test.js
	deleted:    Sprint-2/package-lock.json
	deleted:    Sprint-2/package.json
	deleted:    Sprint-3/package.json
	modified:   Sprint-2/implement/lookup.test.js
	modified:   Sprint-2/implement/lookup.test.js
…uerystring.js

	modified: Added test to check the new function  Sprint-2/implement/querystring.test.js
	modified:   Sprint-2/implement/tally.test.js
…nd getMostFrequentValue for improved readability and maintainability
@sedazam sedazam marked this pull request as ready for review July 10, 2025 22:53
@sedazam sedazam added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Participant to add when requesting review labels Jul 10, 2025
Copy link
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.

Code is good. Good job! I manly have some suggestions.

Comment on lines +14 to +18
queryParams[pair] = "";
} else {
const key = pair.slice(0, firstEqualIndex);
const value = pair.slice(firstEqualIndex + 1);
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.

Please note that in real querystring, both key and value are percent-encoded or URL encoded in the URL. For example, the string "5%" will be encoded as "5%25". So to get the actual value of "5%25" (whether it is a key or value in the querystring), you should call a function to decode it.
May I suggest looking up any of these terms, and "How to decode URL encoded string in JS"?

Copy link
Author

Choose a reason for hiding this comment

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

Character Encoded As Reason
% %25 Starts an encoded sequence, must be escaped itself
(space) %20 Spaces are not allowed in URLs
& %26 Separates query parameters
= %3D Assigns values to keys
? %3F Starts a query string
! %21 Reserved character, often escaped
/ %2F Used in paths, escaped in values
# %23 Starts a URL fragment
" %22 Double quotes must be escaped
' %27 Single quotes can cause parsing issues
: %3A Used in protocols (e.g., http:)
+ %2B May be interpreted as space in form data

Copy link
Contributor

Choose a reason for hiding this comment

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

There are built-in functions you can use to decode the percent-encoded key and value at lines 14 and 18.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 19, 2025
@sedazam sedazam added the Needs Review Participant to add when requesting review label Jul 19, 2025
@sedazam
Copy link
Author

sedazam commented Jul 19, 2025

Merge branch 'main' into Sprint-2

What happened here?

Copy link
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.

  • Changes look good.

  • Additional changes are optional. I will mark this PR as complete first.

  • You are seeing this probably because you have recently included the latest commit made on CYF's main.

image

Comment on lines +34 to +40
const coinValues = {
"1p": 10,
"5p": 6,
"50p": 4,
"20p": 10,
};
const totalAmount = totalTill(till);

Copy link
Contributor

Choose a reason for hiding this comment

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

The variable at line 34 should be kept as till. The property values represent the number of coins.

The coinValues to be used as a lookup table is supposed to represent the value of each coin.
And since the lookup table is used locally within the function, you can write

function totalTillAlternative(till) {
  const coinValues = {
    "1p": 1,
    "5p": 5,
    "50p": 50,
    "20p": 20,
  };
  
  let total = 0;

  // Code to calculate total ...

  return `£${(total / 100).toFixed(2)}`;
}

Comment on lines +20 to +32
function totalTillAlternative(till) {
let total = 0;

for (const [coin, quantity] of Object.entries(till)) {
// Convert coin to a number and multiply by quantity
total += coinValues[coin] * quantity;

if (!coin in coinValues) {
throw new Error(`Coin ${coin} is not recognized`);
}

return `£${(total / 100).toFixed(2)}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't have to delete your original implementation, you could just implement a new function.

Anyway, this totalTillAlternative() is similar to totalTill() (why keep two similar copies?), and it has a bug.

Comment on lines +14 to +18
queryParams[pair] = "";
} else {
const key = pair.slice(0, firstEqualIndex);
const value = pair.slice(firstEqualIndex + 1);
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.

There are built-in functions you can use to decode the percent-encoded key and value at lines 14 and 18.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review Reviewed Volunteer to add when completing a review labels Jul 20, 2025
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 review comments have been addressed 📅 Sprint 2 Assigned during Sprint 2 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants