Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions Sprint-1/fix/median.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,33 @@
// or 'list' has mixed values (the function is expected to sort only numbers).

function calculateMedian(list) {
const middleIndex = Math.floor(list.length / 2);
const median = list.splice(middleIndex, 1)[0];
return median;
if (Array.isArray(list)) {
// [...list] creates a shallow copy
const listNumberOnlyAsc = [...list].filter(
(item) => !isNaN(item) && item !== null
);
Comment on lines +11 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Why clone the original array when using .filter()? (This comment applies to all the code that that uses .filter().)

if (listNumberOnlyAsc == undefined || listNumberOnlyAsc == 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will the condition on line 14 ever be true?

return null;
}
const listPureNumber = [...listNumberOnlyAsc].filter(
(item) => typeof item == "number"
);
Comment on lines +17 to +19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Why not include this filtering condition in the filter on lines 11-13 so that we only need to filter the array once?

  • You may also want to check out the methods in Number to see if there is any suitable method you can use to retain valid numbers.

const listSorted = [...listPureNumber].sort((a, b) => a - b);

if (listSorted.length % 2 === 1) {
const middleIndex = Math.floor(listSorted.length / 2);
const median = [...listSorted].splice(middleIndex, 1)[0]; //code '.splice(middleIndex,1)' means remove the middle index num in array and put it in a new array, '[0]' is the first item in this new array

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use the same syntax you are using on line 30?

return median;
} else {
//retrieve the second middle number from the 2 middle numbers, and use it to retrieve the first middle number amd calculate the median
const secondMiddleIndex = Math.floor(listSorted.length / 2);
const median =
(listSorted[secondMiddleIndex - 1] + listSorted[secondMiddleIndex]) / 2;
return median;
}
} else {
return null;
}
Comment on lines +33 to +35

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could also express

  if (condition) {
    ... // Code to handle normal cases
  } else {
    return null; 
  }

as

  if (!condition) { // Opposite condition
    return null;
  }

  ... // Code to handle normal cases

This way, we don't need to use else. This is possible only if the else part has a return statement.

}

module.exports = calculateMedian;
2 changes: 1 addition & 1 deletion Sprint-1/fix/median.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe("calculateMedian", () => {
expect(list).toEqual([3, 1, 2]);
});

[ 'not an array', 123, null, undefined, {}, [], ["apple", null, undefined] ].forEach(val =>
[ 'not an array', 123, null, undefined, {}, [], ["apple", null, undefined] ].forEach(val =>
it(`returns null for non-numeric array (${val})`, () => expect(calculateMedian(val)).toBe(null))
);

Expand Down
12 changes: 11 additions & 1 deletion Sprint-1/implement/dedupe.js
Original file line number Diff line number Diff line change
@@ -1 +1,11 @@
function dedupe() {}
function dedupe(list) {
if (list.length === 0){
return null;
}else{
Comment on lines +2 to +4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The spacing around else is not consistent. Why not use a formatter to keep the code consistently formatted?

//compare the array elements and check is there is duplicate
listDedupe = list.filter((item, index) => list.indexOf(item) === index);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

listDedupe is not yet declared.

return listDedupe;
}
}

module.exports = dedupe;
22 changes: 21 additions & 1 deletion Sprint-1/implement/dedupe.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,33 @@ E.g. dedupe([1, 2, 1]) returns [1, 2]
// Given an empty array
// When passed to the dedupe function
// Then it should return an empty array
test.todo("given an empty array, it returns an empty array");
describe("dedupe", () => {
it("returns empty array", () => {
expect(dedupe([])).toBe(null)
});

// Given an array with no duplicates
// When passed to the dedupe function
// Then it should return a copy of the original array
[
{ input: ['a','b','c','d','e','f'], expected: ['a','b','c','d','e','f'] },
{ input: [1, 2, 3, 4, 5, 6], expected: [1, 2, 3, 4, 5, 6] },
{ input: ['apple', 'orange', 'grape'], expected: ['apple', 'orange', 'grape'] },
{ input: [12, 23, 34, 45, 56, 67], expected: [12, 23, 34, 45, 56, 67] },
].forEach(({ input, expected }) =>
it(`returns the original list with no duplicates [${input}]`, () => expect(dedupe(input)).toEqual(expected))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current test checks only if both the original array and the returned array contain identical elements.
In order to validate the returned array is a different array, we need an additional check.

Can you find out what this additional check is?

);

// Given an array of strings or numbers
// When passed to the dedupe function
// Then it should return a new array with duplicates removed while preserving the
// first occurrence of each element from the original array.
[
{ input: ['a','a','a','b','b','c'], expected: ['a','b','c'] },
{ input: [5, 1, 1, 2, 3, 2, 5, 8], expected: [5, 1, 2, 3, 8] },
{ input: [1, 2, 1], expected: [1, 2] },
{ input: ['apple', 'banana', 'apple', 'banana'], expected: ['apple', 'banana'] },
].forEach(({ input, expected }) =>
it(`returns the deduplicated list [${input}]`, () => expect(dedupe(input)).toEqual(expected))
);
});
14 changes: 14 additions & 0 deletions Sprint-1/implement/max.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,18 @@
function findMax(elements) {
const elementsNumberOnly = [...elements].filter((element) => !isNaN(element));
const elementsPureNumber = [...elementsNumberOnly].filter(
(item) => typeof item == "number"
);
Comment on lines +2 to +5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could these filters be combined so that the code only needs to filter the array once?


if (elementsPureNumber.length === 0) {
return -Infinity;
} else if (elementsPureNumber.length === 1) {
return elementsPureNumber[0];
} else if (elementsPureNumber.length > 1) {
const elementsSorted = elementsPureNumber.sort((a, b) => a - b);
const elementsMax = elementsSorted.toSpliced(0, elementsSorted.length - 1);
return elementsMax[0];
Comment on lines +12 to +14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • You could also just return the last element of the array without first deleting elements from the array.

  • You could also sort the array in reverse order and access its first element.

}
}

module.exports = findMax;
85 changes: 66 additions & 19 deletions Sprint-1/implement/max.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,75 @@ const findMax = require("./max.js");
// When passed to the max function
// Then it should return -Infinity
// Delete this test.todo and replace it with a test.
test.todo("given an empty array, returns -Infinity");
describe("findMax", () => {
it("returns infinity if input is an empty list", () => {
expect(findMax([])).toBe(-Infinity);
});

// Given an array with one number
// When passed to the max function
// Then it should return that number
// Given an array with one number
// When passed to the max function
// Then it should return that number
[[3], [1], [2], [50], [100], [1]].forEach((val) =>
it(`returns the same number when array with one number is input (${val})`, () =>
expect(findMax(val)).toBe(val[0]))
);

// Given an array with both positive and negative numbers
// When passed to the max function
// Then it should return the largest number overall
// Given an array with both positive and negative numbers
// When passed to the max function
// Then it should return the largest number overall
[
{ input: [30, 50, 10, 40], expected: 50 },
{ input: [5, -50], expected: 5 },
{ input: [-1, 2, 200], expected: 200 },
{ input: [5, -3, -5, -10], expected: 5 },
].forEach(({ input, expected }) =>
it(`returns the maximum number when array contains negative or positive numbers [${input}]`, () =>
expect(findMax(input)).toEqual(expected))
);

// Given an array with just negative numbers
// When passed to the max function
// Then it should return the closest one to zero
// Given an array with just negative numbers
// When passed to the max function
// Then it should return the closest one to zero
[
{ input: [-3, -6], expected: -3 },
{ input: [-500, -50], expected: -50 },
{ input: [-1, -2, -3, -4], expected: -1 },
].forEach(({ input, expected }) =>
it(`returns the closest number to zero when array contains only negative numbers [${input}]`, () =>
expect(findMax(input)).toEqual(expected))
);

// Given an array with decimal numbers
// When passed to the max function
// Then it should return the largest decimal number
// Given an array with decimal numbers
// When passed to the max function
// Then it should return the largest decimal number
[
{ input: [3.1, 3.2], expected: 3.2 },
{ input: [5.1, 50.7, 2.3], expected: 50.7 },
{ input: [-1.1, -2.4, 2], expected: 2 },
{ input: [-15.38, -3.2, -5.01, -10.0], expected: -3.2 },
].forEach(({ input, expected }) =>
it(`returns the maximum number in array contains decimal number [${input}]`, () =>
expect(findMax(input)).toEqual(expected))
);

// Given an array with non-number values
// When passed to the max function
// Then it should return the max and ignore non-numeric values
// Given an array with non-number values
// When passed to the max function
// Then it should return the max and ignore non-numeric values
[
{ input: ["hey", 10, "hi", 60, 10], expected: 60 },
{ input: [9, "orange", 1], expected: 9 },
{ input: [-9, "3", "apple", 1], expected: 1 },
{ input: [9, "grape", "banana"], expected: 9 },
].forEach(({ input, expected }) =>
it(`returns the maximum number when array contains non-number elements and numbers [${input}]`, () =>
expect(findMax(input)).toEqual(expected))
);

// Given an array with only non-number values
// When passed to the max function
// Then it should return the least surprising value given how it behaves for all other inputs
// Given an array with only non-number values
// When passed to the max function
// Then it should return the least surprising value given how it behaves for all other inputs
[["hey", "hi"], ["apple"], ["banana", true]].forEach((input) =>
it(`returns -Infinity when the array contains only non-number elements (${input})`, () =>
expect(findMax(input)).toBe(-Infinity))
);
});
17 changes: 17 additions & 0 deletions Sprint-1/implement/sum.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,21 @@
function sum(elements) {
const elementsNumberOnly = [...elements].filter((element) => !isNaN(element));
const elementsPureNumber = [...elementsNumberOnly].filter(
(item) => typeof item == "number"
);
Comment on lines +2 to +5

Copy link
Copy Markdown
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 from the following function calls (on extreme cases)?
Does your function return the value you expected?

sum([NaN, 1]);
sum([Infinity, -Infinity]);


if (elementsPureNumber.length === 0) {
return 0;
} else if (elementsPureNumber.length === 1) {
return elementsPureNumber[0];
} else if (elementsPureNumber.length > 1) {
const iterator = elementsPureNumber.values();
let sumOfArray = 0;
for (const value of iterator) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for-of loop works on array directly (arrays are iterable). You could just write for (const value of elementsPureNumber) { .

sumOfArray += value;
}
return sumOfArray;
}
Comment on lines +7 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code on lines 13-17 also works for array of any size. If you prefer having less code, you could consider using the same code to handle all cases. That way, you don't need to use if-else.

}

module.exports = sum;
40 changes: 39 additions & 1 deletion Sprint-1/implement/sum.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,62 @@ const sum = require("./sum.js");
// Given an empty array
// When passed to the sum function
// Then it should return 0
test.todo("given an empty array, returns 0")
describe("sum", () => {
it("returns infinity if input is an empty list", () => {
expect(sum([])).toBe(0);
});

// Given an array with just one number
// When passed to the sum function
// Then it should return that number
[[3], [1], [2], [50], [100], [1]].forEach((val) =>
it(`returns the same number when array with one number is input (${val})`, () =>
expect(sum(val)).toBe(val[0]))
);

// Given an array containing negative numbers
// When passed to the sum function
// Then it should still return the correct total sum
[
{ input: [10, 20, 30], expected: 60 },
{ input: [5, -50], expected: -45 },
{ input: [-100, 200], expected: 100 },
{ input: [1, -1, -2, 2], expected: 0 },
].forEach(({ input, expected }) =>
it(`returns the sum when array contains negative or positive numbers [${input}]`, () =>
expect(sum(input)).toEqual(expected))
);

// Given an array with decimal/float numbers
// When passed to the sum function
// Then it should return the correct total sum
[
{ input: [1.1, 1.2], expected: 2.3 },
{ input: [-1.1, -2.4, 1.1], expected: -2.4 },
{ input: [-15.38, -3.2, -5.01, -10.0], expected: -33.59 },
].forEach(({ input, expected }) =>
it(`returns the sum in array contains decimal number [${input}]`, () =>
expect(sum(input)).toEqual(expected))
);
Comment on lines +46 to +52

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Decimal numbers in most programming languages (including JS) are internally represented in "floating point number" format. Floating point arithmetic is not exact. For example, the result of 46.5678 - 46 === 0.5678 is false because 46.5678 - 46 only yield a value that is very close to 0.5678. Even changing the order in which the program add/subtract numbers can yield different values.

So the following could happen

  expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.805 );                // This fail
  expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.8049999999999997 );   // This pass
  expect( 0.005 + 0.6 + 1.2 ).toEqual( 1.8049999999999997 );   // This fail

  console.log(1.2 + 0.6 + 0.005 == 1.805);  // false
  console.log(1.2 + 0.6 + 0.005 == 0.005 + 0.6 + 1.2); // false

Can you find a more appropriate way to test a value (that involves decimal number calculations) for equality?

Suggestion: Look up

  • Checking equality in floating point arithmetic in JavaScript
  • Checking equality in floating point arithmetic with Jest


// Given an array containing non-number values
// When passed to the sum function
// Then it should ignore the non-numerical values and return the sum of the numerical elements
[
{ input: ["hey", 10, "hi", 60, 10], expected: 80 },
{ input: [9, "orange", 1], expected: 10 },
{ input: [-9, "3", "apple", 1], expected: -8 },
{ input: [9, "grape", "banana"], expected: 9 },
].forEach(({ input, expected }) =>
it(`returns the sum in array contains decimal number [${input}]`, () =>
expect(sum(input)).toEqual(expected))
);

// Given an array with only non-number values
// When passed to the sum function
// Then it should return the least surprising value given how it behaves for all other inputs
[["hey", "hi"], ["apple"], ["banana", true]].forEach((input) =>
it(`returns 0 when the array contains only non-number elements (${input})`, () =>
expect(sum(input)).toBe(0))
);
});
Loading