Skip to content

ITP|25|LONDON|LOVETH_OKAFOR|Module-Data-Groups|WEEK2_SPRINT_2 #320

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 24 commits into
base: main
Choose a base branch
from

Conversation

loveth900
Copy link

@loveth900 loveth900 commented Mar 4, 2025

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 COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • 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.

@loveth900 loveth900 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 4, 2025
@loveth900 loveth900 changed the title ITP|25|LONDON|LOVETH ITP|25|LONDON|LOVETH_OKAFOR|Module-Data-Groups|WEEK2_SPRINT_2 Mar 4, 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.

There seem to be some mixed up in terms of which files should contain function definition and which files should contain test code. Can you move the function definitions to the proper files and implement the Jest test in the .test.js files?

Comment on lines 36 to 45
function contains(obj, property) {
// Check that the input is an object, not an array.
if (typeof obj !== 'object' || obj === null || Array.isArray(obj)) {
return false;

}

// we need to verify if the specified property is present in the object.
return property in oby
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this function be defined in contains.js?

Also, consider the following two approaches for determining if an object contains a property:

  let obj = {}, propertyName = "toString";
  console.log( propertyName in obj );                // true
  console.log( obj.hasOwnProperty(propertyName) );   // false

Which of these approaches suits your needs better?
For more info, you can look up JS "in" operator vs hasOwnProperty.

Copy link
Author

Choose a reason for hiding this comment

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

i have fixed it properly, thanks for the correction

// we need to verify if the specified property is present in the object.
return property in oby
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You were supposed to implement Jest test in this files according to the description in the comments.

Copy link
Author

Choose a reason for hiding this comment

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

hi dear mentor i have implemented as instructed.

Comment on lines 1 to 16
function createLookup(arr, key) {
const lookup = {};
// initialize an empty object to store the lookup table

// loop through each object in the input array.
Array.forEach(item => {
// assign the value of the specified key from the object as the lookup key.
// and store the entire object as the corresponding value
lookup[item[key]] = item;

})

return lookup;
// needed to return the completed lookup table.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

The function defined in lookup.test.js should replace this one.

Copy link
Author

Choose a reason for hiding this comment

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

alright, i have fixed the function.

Comment on lines 35 to 53
function createLookup(countryCurrencyPairs) {
const createLookup = {};
// create an empty object to hold the key-value pairs

// loop through each country-currency pair in the array
countryCurrencyPairs.forEach(pair => {
const [countryCode, currencyCode] = pair;
// Destructure each pair into countryCode and currencyCode
lookup[countryCode] = currencyCode;
// store the country code as the key and currency code as the value

});

return lookup;

// return the constructed lookup object

}

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is correct but it should be defined in lookup.js.

You should only include Jest test code in this file.

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines +8 to +26
function parseQueryString(queryString) {
const params = {};

// split the query string by '&' to separate different key-value pairs
const pairs = queryString.split("&");

pairs.forEach(pair => {

// split each into key and value by the first '=' sign

const{key, value} = pair.split("=").map(decodeURIComponent);

// if there is more than one '=' symbol, join the rest of the value together
if (value && value.includes("=")) {
params[key] = pair.slice(key.length + 1);
// keep the value intact
} else {
params[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.

  1. This should be implemented in querystring.js.

  2. For each of the following function calls, does your function return the value you expect?

parseQueryString("a=b&=&c=d")
parseQueryString("a=")
parseQueryString("=b")
parseQueryString("a=b&&c=d")
parseQueryString("a%25b=c%26d")
parseQueryString("abc=x=y%26=z")
parseQueryString("a%25b=x=y=z")

Note: the %25 and %26 are URL encoded or percent encoded characters.

Comment on lines 41 to 44
test("tally throws an error when passed a string", () => {
expect(() => tally("invalid"))
.toThrow("Input must be an array");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does your function pass this test?

Copy link
Author

Choose a reason for hiding this comment

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

yes it's working now, thanks for letting me know.

Comment on lines 19 to 22

// a) What is the current return value when invert is called with { a : 1 }

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

Choose a reason for hiding this comment

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

Why not answering these questions?

Copy link
Author

Choose a reason for hiding this comment

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

thanks dear mentor i have answered the question.

Comment on lines 3 to 12
test("", () => {
const obj = { x: 10, y: 20 };
const answer = invert(obj);
expect(answer).toEqual({ 10: "x", 20: "y" });
});
test("", () => {
const obj = { name: "Evelyn", Age: 35 };
const answer = invert(obj);
expect(answer).toEqual({ Evelyn: "name", 35: "Age" });
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these two tests trying to test?

Copy link
Author

Choose a reason for hiding this comment

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

well i think there are trying to test the the behavior of the invert function, which takes an object as input and swaps its keys and values.

Comment on lines -33 to +40
return maxFreq === 0 ? NaN : mode;
return modeValue;
Copy link
Contributor

@cjyuan cjyuan Mar 11, 2025

Choose a reason for hiding this comment

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

Do you know in what circumstance the value returned by this function is not the same as the value returned by the original function?

Copy link
Author

Choose a reason for hiding this comment

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

Yes i have an idea, the two return values are different when there is no mode in the dataset (i.e., when maxFreq === 0), because the first return statement returns NaN, whereas the second return statement could return an incorrect modeValue.

return mode;
}

module.exports = calculateMode;
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should only contain Jest test code.

The definition of the function and module.exports = calculateMode; should be placed in mode.js instead.

Copy link
Author

Choose a reason for hiding this comment

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

Hi dear mentor @cjyuan have corrected the error i will appriciate if you check my work

@cjyuan cjyuan 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 Mar 12, 2025
@loveth900 loveth900 added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Mar 22, 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 all review comments have been addressed. 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