London | May-2025 | KhilolaRustamova | Sprint 2 | Sprint 2 coursework by HilolaRustam · Pull Request #589 · CodeYourFuture/Module-Data-Groups

HilolaRustam · 2025-07-12T14:58:23Z

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.

Questions

Ask any questions you have for your reviewer.

AdnanGondal · 2025-07-19T10:59:48Z

Sprint-2/debug/recipe.js

Great explanations and solutions for the debug section - well done.

AdnanGondal · 2025-07-19T11:14:45Z

Sprint-2/implement/lookup.test.js

Perhaps it would be better to add different edge cases to these tests such as what happens when:

Empty input

When there are duplicate keys

When a pair has extra values, such as createLookup([["UK", "GBP", "Extra"]])).

i added some more test cases.

These tests are really interesting - it feels like there are three things we could do with ["UK", "GBP", "Extra"]:

Ignore the "Extra"

Ignore the whole entry because it's not in the expected format

Throw an error because the caller probably didn't something wrong and we want to tell them there's a problem

Why did you pick 1? How do you feel about 2 and 3?

I thought 1 is flexible and tolerant of inputs with extra data.
2 would not be so helpful as it would ignore the data and not alert the data issue.
And 3 would be the choice if this function was part of a stricter API where input validation is important to make sure data issues are caught early.

Yeah, 1 vs 3 both sound pretty reasonable. There are trade-offs between ignoring potentially bad data vs erroring. e.g. imagine if someone had intentionally passed three values because a country had two currencies:

["ZW", "ZiG", "USD"]

If we proactively error, we tell them "Our system only supports two currencies, we're ignoring one of the ones you passed, you probably don't want to pass two, pick one". By not erroring, developers may assume their code is working with multiple currencies, when really some is being ignored.

AdnanGondal · 2025-07-19T11:29:14Z

Sprint-2/implement/querystring.test.js

A great set of test cases. Just had one question.
What do you expect to happen when you run parseQueryString("a=1&&b=2").
Can you add a test case for it?

test is going to look like this.
test("parses multiple key-value pairs", () => {
expect(parseQueryString("a=1&&b=2")).toEqual({ a: "1", "", b: "2" });
});

AdnanGondal

A great submission - but I have added some comments regarding additional test cases. Once done, let me know and I will mark as Complete.

illicitonion

This is looking really good - just a couple of small comments :)

illicitonion · 2025-08-13T15:49:52Z

Sprint-2/implement/querystring.js

-    queryParams[key] = value;
-  }
+   const [key, ...rest] = pair.split("="); // Grab everything after first '='
+   const value = rest.join("="); // Safely join back if value had '=' signs


This joining logic is really useful, but if you removed it none of your tests would start failing. Could you add a test that shows why this logic is here?

i did, thank you

illicitonion · 2025-08-13T15:51:46Z

Sprint-2/implement/lookup.test.js

These tests are really interesting - it feels like there are three things we could do with ["UK", "GBP", "Extra"]:

Ignore the "Extra"

Ignore the whole entry because it's not in the expected format

Throw an error because the caller probably didn't something wrong and we want to tell them there's a problem

Why did you pick 1? How do you feel about 2 and 3?

HilolaRustam added 9 commits July 9, 2025 14:14

address.js is debuged

eb4db92

author.js file debuged and tested

8cfea90

recipe.js file debugged and tested

b7a8e42

added explanation

6da782c

contains function made and tested

95b65e7

lookup function is done and tested

f721233

querystring.js bug is fixed and tested

95a40d5

tally.js function made and tested

499b60a

invert.js fixed and tested

4d3c115

HilolaRustam 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. labels Jul 12, 2025

AdnanGondal self-requested a review July 19, 2025 10:58

AdnanGondal added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 19, 2025

AdnanGondal reviewed Jul 19, 2025

View reviewed changes

Sprint-2/debug/recipe.js

Copy link

AdnanGondal Jul 19, 2025

Choose a reason for hiding this comment

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

Great explanations and solutions for the debug section - well done.

AdnanGondal reviewed Jul 19, 2025

View reviewed changes

AdnanGondal suggested changes Jul 19, 2025

View reviewed changes

AdnanGondal added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 19, 2025

added new tests

088b220

HilolaRustam 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 Jul 25, 2025

HilolaRustam requested a review from AdnanGondal August 11, 2025 19:42

illicitonion reviewed Aug 13, 2025

View reviewed changes

illicitonion 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 13, 2025

added test containing equal signs

0a95ae9

HilolaRustam 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 Aug 13, 2025

illicitonion 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 Aug 14, 2025

This comment was marked as resolved.

Sign in to view

This was referenced Aug 14, 2025

Module Data Groups Khilola Rustamova #768

Closed

London | 25-ITP-May | Module Data Groups |Khilola Rustamova HilolaRustam/My-Coursework-Planner#53

Open

illicitonion closed this Sep 1, 2025

Uh oh!

Conversation

HilolaRustam commented Jul 12, 2025

Learners, PR Template

Changelist

Questions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdnanGondal left a comment

Choose a reason for hiding this comment

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants