London | May-2025 | KhilolaRustamova | Sprint 2 | Sprint 2 coursework #589
Conversation
HilolaRustam
added
📅 Sprint 2
AdnanGondal
added
Review in progress
There was a problem hiding this comment.
Great explanations and solutions for the debug section - well done.
There was a problem hiding this comment.
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"]])).
There was a problem hiding this comment.
i added some more test cases.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
left a comment
AdnanGondal
left a comment
There was a problem hiding this comment.
A great submission - but I have added some comments regarding additional test cases. Once done, let me know and I will mark as Complete.
AdnanGondal
added
Reviewed
HilolaRustam
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is looking really good - just a couple of small comments :)
| queryParams[key] = value; | ||
| } | ||
| const [key, ...rest] = pair.split("="); // Grab everything after first '=' | ||
| const value = rest.join("="); // Safely join back if value had '=' signs |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
illicitonion
added
Reviewed
HilolaRustam
added
Needs Review
illicitonion
added
Complete
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as resolved.
This comment was marked as resolved.
illicitonion
changed the title
3 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.