Skip to content

Conversation

@alexandru-pocovnicu
Copy link

@alexandru-pocovnicu alexandru-pocovnicu commented Nov 3, 2025

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

implemented functions using TDD

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@alexandru-pocovnicu alexandru-pocovnicu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 3, 2025
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

4 similar comments
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

Copy link

@takanocap takanocap left a comment

Choose a reason for hiding this comment

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

@alexandru-pocovnicu Good effort on Sprint 2. However, there a few comments for you to address.

console.log(`${recipe.title}, serves ${recipe.serves}, ingredients:`);

for (const ingredient of recipe.ingredients) {
console.log(ingredient);

Choose a reason for hiding this comment

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

can this be done with an array method which may be simpler/apt in this scenario?

if (typeof obj !== "object" || obj === null || Array.isArray(obj)) {
throw new Error("Parameter is not an object literal");
}
if (Object.keys(obj).length === 0) {

Choose a reason for hiding this comment

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

is there a way to make fewer checks so we you don't repeat the same check logically?

return true;
}

return false;

Choose a reason for hiding this comment

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

is there a way we can reduce repetition? what is the DRY principle?

// implementation here
function createLookup(countryCurrency) {
let countryCurrencyPairs = {};
for (const pair of countryCurrency) {

Choose a reason for hiding this comment

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

what happens if you test this solution with an argument which is not an object?

@@ -1,16 +1,32 @@
function parseQueryString(queryString) {
const queryParams = {};
if (queryString.length === 0) {

Choose a reason for hiding this comment

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

is there a better way to check this and exit or handle the error if its not a valid querystring?

for (const pair of keyValuePairs) {
const [key, value] = pair.split("=");
queryParams[key] = value;
if(pair===""){

Choose a reason for hiding this comment

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

can this be shorter or tested in a different way ? investigate how to test if string has a empty or falsy value

Choose a reason for hiding this comment

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

so should i change my tests also? if i use array destructuring none of the test are passing anymore

Choose a reason for hiding this comment

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

yes please because thats what will typically happen in a work environment

Choose a reason for hiding this comment

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

if i use array destructuring it doesn't pass the tests as some of the values have "=" in them, or at least i don't know how to do it. some pointers would be much appreciated , thank you

Comment on lines +22 to +24
test("if the object contains the property, return 'true'", () => {
expect(contains({ a: 1, "a,s,3": "op" }, ["a", "s", 3])).toEqual(true);
});
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is slightly surprising behaviour - the object doesn't really contain that array, it contains a stringification of that array. But this works, so I guess it is ok to test for...

I would probably use a different name for the test, though, to make it clear how it's different from the test on line 19.

Comment on lines +13 to +15
test("throw error if the argument is not an object", () => {
expect(() => createLookup("notAnObject")).toThrow(
"Argument must be an array"
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit odd that the test name here talks about objects, the string says notAnObject, but the error message is about being an array?

//[[ "a", 1], ["b", 2]]

// c) What is the target return value when invert is called with {a : 1, b: 2}
//{"1":a,"2":b}
Copy link
Member

Choose a reason for hiding this comment

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

What is the type of a and b here? Are they strings/numbers/booleans/variables/something else?

@illicitonion 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 Dec 19, 2025
@github-actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

1 similar comment
@github-actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above.

@alexandru-pocovnicu alexandru-pocovnicu closed this by deleting the head repository Jan 1, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants