Skip to content

ZA Cape Town | ITP-May-2025 | Dawud Vermeulen | Sprint 2 | Module-Data-Groups #755

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

Conversation

Dave-Vermeulen
Copy link

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 | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

completed the tasks within:

  • Debug
  • Implement
  • Interpret

Questions

Please review my PR 🙏🏽

@Dave-Vermeulen Dave-Vermeulen added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 9, 2025
@cjyuan cjyuan added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 9, 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.

  • You missed updating the .test.js files

Comment on lines +3 to +10
if(obj === null || obj === undefined){
return false
}
return obj[property] !== undefined;
}

module.exports = contains;
// work done.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you expect the from the following function calls?

contains("ABC", "1");
contains(["A", "B", "C"], "1");
contains(123, "1");
contains(true, "1");
contains({ key: undefined }, "key");

Suggestion:, Look up JS "in" operator vs Object.hasOwn.

Comment on lines +11 to 13
const key = parts[0];
const value = parts.length > 1 ? parts.slice(1).join('=') : ''; //clunky but it works to split them and stitch it together again
queryParams[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.

Please note that in real querystring, both key and value are percent-encoded or URL encoded in the URL. For example, the string "5%" will be encoded as "5%25". So to get the actual value of "5%25" (whether it is a key or value in the querystring), you should call a function to decode it.
May I suggest looking up any of these terms, and "How to decode URL encoded string in JS"?

Comment on lines +1 to +11
function tally(items) {
if (!Array.isArray(items)){
throw new TypeError('that aint no array bay bay'); //okay so this is the biggie smalls and its working
}
const counts = {};

for (const item of items){
counts[item] = (counts[item] || 0) +1; //simple sexy core logic
}
return counts;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the following function call returns the value you expect?

tally(["toString", "toString"]);

Suggestion: Look up an approach to create an empty object with no inherited properties.

@@ -10,20 +10,30 @@ function invert(obj) {
const invertedObj = {};

for (const [key, value] of Object.entries(obj)) {
invertedObj.key = value;
const numKey = Number(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to convert the key to a number when it looks like a number.

Comment on lines +29 to +31
// {'1':'a'}
// b) What is the current return value when invert is called with { a: 1, b: 2 }
// { '1': 'a', '2': 'b'}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the expected return value from the original code, not the actual return value.

Comment on lines 36 to +37
// d) Explain why the current return value is different from the target output

// cause its converted to strings, the key must be a int and the value must be a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

The question is asking, why with the original code, invert({a: 1}) does not return {'1': a} (or {1:a}).

Note: The property name (key) is always a string. Even when we express the key as a number (e.g., { 1 : 'a' }), the key will still be converted to a string '1'.

@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. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 9, 2025
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