Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Groups/blob/general-review-feedback/Sprint-1/feedback.md
Doing so can help reviewers speed up the review process. Thanks.
Can you update PR Title and Description according to this PR Guide in order to pass the automated "Validate PR Metadata" check?
It is never too early to develop the habit to keep the code clean by removing (or at least commenting out) unused code or debugging code.
Array is passed to a function by reference (instead of by value). This implies when an array is passed to a function via parameter, the array is shared by the caller and the callee (the function being called).
To avoid modifying the caller's array, a common practice is to make a clone of the array (that's passed as a parameter) and modify the clone in the callee instead.
Do you plan to consider also -Infinity, Infinity, and NaN in the median calculation?
Do you plan to consider also -Infinity, Infinity, and NaN in the median calculation (and in the functions in implement/max.js and implement/sum.js)?
If you want to exclude Infinity and NaN, consider using the built-in function Number.isFinite().
- Naming convention for variables storing an array is:
- Use plural form (e.g.
numbers), or - Append a suffix
ArrayorListto the name (e.g.,numberArray)
- Use plural form (e.g.
numbers is no longer the original array and it is not needed later.
So we could improve the performance by not making another copy of the array and write the code on line 17 as
const sorted = numbers.sort((a, b) => a - b); // sorted and numbers are the same arrayNote: sorted is a more meaningful name so it is good to keep it.
The arrays specified in median.test.js are already sorted by coincidence.
Typically, to find the median value of an array, we first need to sort it. However, to prevent modifying list directly, it's best to work with a cloned version instead.
Could you add one or more statements to create a cloned copy of list and then sort the numbers in the cloned array?
The elements in [1, 2, 3] are already sorted. If we use this array as a test value, it will be difficult to determine whether the function modifies the input or not (by sorting the given array).
We must use .toEqual() instead of .toBe() when comparing arrays or objects; .toBe() does not compare two arrays by their values. Since toEqual() works equally well on primitive values such as number, and string, you can also just use .toEqual() to compare all types of values.
In JS, an array is a kind of object, and objects are compared by references instead of by values.
To learn more about how arrays/objects are compared/copied in JS, and to learn more about how to compare two arrays by their values, may I suggest asking ChatGPT
- Why in JS
[1, 2, 3] === [1, 2, 3]isfalse?" - How to compare two arrays by value in JS?
This test should fail if the function returns the original array (instead of a copy of the original array).
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?
There is a chance that, even though result has incorrect elements (for example, []),
the two tests could still pass. Can you figure out why, and then fix the tests accordingly?
What do you expect from the following function calls (on extreme cases)? Does your function return the value you expected?
findMax([-Infinity])
findMax([-Infinity, 0])
findMax([NaN])
findMax([0, NaN, 1])
Inifinity is a keyword in the JavaScript language. When an array is empty, the value to be returned
should be be -Infinty, not a string value.
When a string representing a valid numeric literal (for example, "300") is compared to a number,
JavaScript first converts the string into its numeric equivalent before performing the comparison.
As a result, the expression 20 < "300" evaluates to true.
To test if the function can correctly ignore non-numeric values,
consider including a string such as "300" in the relevant test cases.
Why return a string (instead of a value of type "number") when the given array contains only non-number values?
When a function has a dual return type, it becomes unclear what the caller should expect. Developers would need to look at the implementation or documentation to understand the behavior.
In practice, we write tests not only to verify our current implementation, but also to ensure that future changes do not alter the function's expected behavior.
Currently the following function could also pass this test:
function findMax(elements) {
const onlyNumbers = elements.map(Number).filter((el)=>!Number.isNaN(el));
return Math.max(...onlyNumbers);
}
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]);
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); // falseCan 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
This optional exercise requires reading numbers from a file named input.txt, and then sum up the numbers.
Suggestion: Look up Node.js File system (fs) API.
With the file path specified as an absolute path, the script probably cannot find input.txt when it is executed on a different computer.
Assuming input.txt will always be in the same folder as the script, how would you modify your code to read data from this input.txt file?
Part 2 spec: https://adventofcode.com/2018/day/1#part2