Cape Town | 26-ITP-May| Enice Mutanda| Sprint 2| Data objects - #1419
Cape Town | 26-ITP-May| Enice Mutanda| Sprint 2| Data objects#1419Enice-Codes wants to merge 4 commits into
Conversation
| test("creates a country currency code lookup for multiple codes", () => { | ||
| const countryCurrencyPairs = [["US", "USD"], ["CA", "CAD"]]; | ||
| const result = createLookup(countryCurrencyPairs); | ||
| expect(result).toEqual({ US: "USD", CA: "CAD" }); | ||
| }); |
There was a problem hiding this comment.
I would consider having a smaller starting test for the valid input, to build the test suite up more gradually, prove the function can take different inputs and isn't hardcoded
| test("tally on an array with duplicate items returns counts for each unique item", () => { | ||
| expect(tally(["a"])).toEqual({ a: 1 }); | ||
| expect(tally(["a", "a", "a"])).toEqual({ a: 3 }); | ||
| expect(tally(["a", "a", "b", "c"])).toEqual({ a: 2, b: 1, c: 1 }); | ||
| }); |
There was a problem hiding this comment.
Repeated test as the below - remove and replace with the test described in the comment
| test("tally on an array with duplicate items returns counts for each unique item", () => { | ||
| expect(tally(["a"])).toEqual({ a: 1 }); | ||
| expect(tally(["a", "a", "a"])).toEqual({ a: 3 }); | ||
| expect(tally(["a", "a", "b", "c"])).toEqual({ a: 2, b: 1, c: 1 }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I would argue these are three different behaviours - how might you split them up in the test suite?
| test("tally on an array with duplicate items returns counts for each unique item", () => { | ||
| expect(tally(["a"])).toEqual({ a: 1 }); | ||
| expect(tally(["a", "a", "a"])).toEqual({ a: 3 }); | ||
| expect(tally(["a", "a", "b", "c"])).toEqual({ a: 2, b: 1, c: 1 }); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
Again a repeated test - remove and replace with valid test for the given commented behaviour
Poonam-raj
left a comment
There was a problem hiding this comment.
Missing debug elements here - can you include it in this PR?
I have a few things I'd like you to take another look at, a few answers and explanations are missing. Some tests are also wrong (tally).
Some good logic choices, and methods used.
This comment has been minimized.
This comment has been minimized.
7c1b1ce to
bccf5d7
Compare
| // } | ||
|
|
||
| // a) What is the current return value when invert is called with { a: 1 }? | ||
| // -> { key: 1, "1": "a" } |
There was a problem hiding this comment.
Not quite
The original code read as this
function invert(obj) {
const invertedObj = {};
for (const [key, value] of Object.entries(obj)) {
invertedObj.key = value;
}
return invertedObj;
}
Can you give me a corrected answer based on the original code here?
| // b) What is the current return value when invert is called with { a: 1, b: 2 }? | ||
| // -> { key: 2, "1": "a", "2": "b" } | ||
| // Each loop iteration overwrites the same literal "key" property, so | ||
| // only the last value assigned to it survives - here, 2 from { b: 2 }. |
There was a problem hiding this comment.
If each iteration overwrites to the same literal "key" property take another look at your answer - { key: 2, "1": "a", "2": "b" }
Your answer isn't quite right here
| // -> The bug is `invertedObj.key = value`. Because "key" is written as a | ||
| // literal property name (dot notation), it always sets a property | ||
| // called "key" rather than using the value of the loop variable | ||
| // `key`. Only bracket notation - invertedObj[key] - would use the | ||
| // variable's actual value as the property name. This line also isn't | ||
| // needed at all for a correct inversion; it should be removed. |
Poonam-raj
left a comment
There was a problem hiding this comment.
Just some last bits where your explanation doesn't quite match up
Self checklist
Changelist
tested and passed all my code locally
modified the codes
added code to most dependent file
fix codes on certain exercises and implemented code too