Skip to content

Conversation

@Della-Bella
Copy link

@Della-Bella Della-Bella commented Dec 16, 2025

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

Hi Volunteer,

Please ignore my first commits on this PR. I have tried to clean it up, but for some reason, it is not working, and I will go back to it.

The commits to be revised here are from 'Commits on Dec 16, 2025 - Sprint 2- Lookup.js' "d7ce32bf522dd240741bac5ce9bfc5cc6845e3ae".

Please ignore my first comments on this PR. I have tried to clean but for some reason it is not working, and I will go back to it.

The commits to be revised here are from Commits on Dec 16, 2025 - Sprint 2- Lookup.js
"d7ce32bf522dd240741bac5ce9bfc5cc6845e3ae"

thank you for yout time

@Della-Bella Della-Bella added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 16, 2025
@github-actions
Copy link

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).

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

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 16, 2025
@github-actions
Copy link

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).

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

@Della-Bella Della-Bella added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 17, 2025
@github-actions
Copy link

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).

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

@github-actions github-actions bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 17, 2025
@Della-Bella Della-Bella changed the title Sprint 2 done. New implement/ interpret London | 25-ITP-Sept | Gislaine Della Bella | Sprint 2 | Sprint 2 -Final Dec 17, 2025
@Della-Bella Della-Bella added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Dec 17, 2025
@illicitonion illicitonion 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 Dec 19, 2025
@@ -1,3 +1,11 @@
function contains() {}
function contains(someObject, propertyName) {
let whatTheRobotFound = someObject[propertyName];
Copy link
Member

Choose a reason for hiding this comment

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

What's this variable for?

Copy link
Author

Choose a reason for hiding this comment

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

I realize now that I’m not actually using this variable, so I’ll remove it to keep the code clean.

function contains() {}
function contains(someObject, propertyName) {
let whatTheRobotFound = someObject[propertyName];
if (someObject[propertyName] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

We can store the value undefined in objects. What would you expect to be returned in this case? What does your code do?

contains({"name": undefined}, "name");

Copy link
Author

Choose a reason for hiding this comment

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

I see...my code returns false even if the property exists but is set to undefined. I’ll use the in operator to fix this so it checks for the property instead of its value

function contains(someObject, propertyName) {
let whatTheRobotFound = someObject[propertyName];
if (someObject[propertyName] === undefined) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

You should never have to write:

if (something) {
    return true;
} else {
    return false;
}

Because the condition in an if statement evaluates to a boolean, you can just return it directly:

return something;

which does the same. If you need to negate it, you can

return !something;

// Then it should return false
test.todo("contains on empty object returns false");
test("contains on empty object returns false", () => {
const inputEmpty = [];
Copy link
Member

Choose a reason for hiding this comment

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

This isn't an empty object, it's an array. It may be interesting to test what happens when passed an array, but it's not quite what the test name describes.

Copy link
Author

Choose a reason for hiding this comment

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

I used the symbol for an array [] by mistake. I’ll change it to {}

Comment on lines +45 to +49
test("give a object with proprieties when pass contains with non-exist proprity name it should return false", () => {
const inputProprieties2 = ["f", "g", "h"];
const result = contains(inputProprieties2);
expect(result).toEqual(false);
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure what this test description is trying to convey, or what the test is trying to test... Can you explain?

Copy link
Author

Choose a reason for hiding this comment

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

I meant to pass an object (like {a: 1}) and a property name to check if it exists. I error used an array instead

Comment on lines +55 to +56
const invalidParameters = ("f", "g", "h");
const result = contains(invalidParameters);
Copy link
Member

Choose a reason for hiding this comment

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

Not specifying a second parameter is an interesting case here, but the test name isn't very descriptive of this... Also, what is the type of invalidParameters here?

Copy link
Author

Choose a reason for hiding this comment

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

It is a string. Because I used () instead of square brackets, JavaScript only looks at the last item. I also forgot the second parameter. I’ll fix it to be a proper test

Comment on lines +4 to +5
for (let i = 0; i < countryCurrencyPairs.length; i++) {
const countryCode = countryCurrencyPairs[i][0];
Copy link
Member

Choose a reason for hiding this comment

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

This works, but probably isn't the most clear way of iterating across this array.

Can you think how other looping constructs, or destructuring, could help you make this more clear and easy to read?

Copy link
Author

Choose a reason for hiding this comment

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

i ffor...of loop would work better there.

expect(parseQueryString("equation=x=y+1")).toEqual({ equation: "x=y+1" });
});

// Special characters
Copy link
Member

Choose a reason for hiding this comment

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

What's special about %20?

Copy link
Author

Choose a reason for hiding this comment

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

It’s the code for a space

Comment on lines 19 to +23
// a) What is the current return value when invert is called with { a : 1 }
// 1

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

Choose a reason for hiding this comment

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

I don't think the function currently returns numbers. Can you talk through why you think it does?

Copy link
Author

Choose a reason for hiding this comment

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

I was looking at the values inside the object instead of the object I see now that invert should return a new object with keys and values swapped, like { 1: 'a' } instead of just the number 1..


// d) Explain why the current return value is different from the target output

// The bug is invertedObj.key = value — this literally creates a property named "key"
Copy link
Member

Choose a reason for hiding this comment

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

Good identification of the problem and the fix!

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

Your self-checklist isn't currently correctly filled out - you need to tick boxes with [x] with no spaces around the x.

On the first commits - you should be able to remove with any of:

  1. git rebase -i (see https://mlops.systems/posts/2023-04-28-removing-git-commits.html)
  2. git revert git-commit (replacing git-commit with each git commit hash you want to remove, one at a time, starting from the newest one you want to undo)
  3. Creating a new branch from main, and git cherry-pick git-commit (replacing git-commit with each git commit hash that you want to keep, one at a time, starting from the oldest one you want to keep) - if you do this, you'll need to create a new PR from that branch, and close this one.

@Della-Bella
Copy link
Author

thnk you Daniel for your time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants