Code Review Asked by Syed Mohammad Sannan on November 17, 2021
We have published a product today called MMMR.js this product is licensed under the MIT license and is built by Syed Mohammad Sannan.
The uniqueness of this product is that it is built with and for Javascript though there are many libraries out there such as NumPY, SciPY, and much much more but they are built for Python or another language and there isn’t one for JavaScript that is where MMMR.js comes.
here.
This is the source code of MMMR.js:
// Mean - The average value.
// Median - The mid point value.
// Mode - The most common value.
// Range The smallest and the biggest value.
// This is Mean.
// Mean returns the average value from an array of numbers.
// To call/run this function we can type mean(Your array of numbers separated with commas ',');
const mean = (numbers) => {
let total = 0,
i;
for (i = 0; i < numbers.length; i += 1) {
total += numbers[i];
}
return total / numbers.length;
};
// This is Median.
// Median returns the middle point value from an array of numbers.
// To call/run this function we can type median(Your array of numbers separated with commas ',');
const median = (numbers) => {
const sorted = numbers.slice().sort((a, b) => a - b);
const middle = Math.floor(sorted.length / 2);
if (sorted.length % 2 === 0) {
return (sorted[middle - 1] + sorted[middle]) / 2;
}
return sorted[middle];
};
// This is Mode.
// Mode returns the most common/used value from an array of string, numbers, etc.
// To call/run this function we can type mode(your array of numbers);
const mode = (a) => {
a.sort((x, y) => x - y);
let bestStreak = 1;
let bestElem = a[0];
let currentStreak = 1;
let currentElem = a[0];
for (let i = 1; i < a.length; i++) {
if (a[i - 1] !== a[i]) {
if (currentStreak > bestStreak) {
bestStreak = currentStreak;
bestElem = currentElem;
}
currentStreak = 0;
currentElem = a[i];
}
currentStreak++;
}
return currentStreak > bestStreak ? currentElem : bestElem;
};
// This is Range.
// Range is basically a function used to return the smallest and the biggest values from an array of numbers.
// To call/run this function we can type range(your array of numbers);
const range = (value) => {
value.sort();
return [value[0], value[value.length - 1]];
}
We just wanted you to review this code and tell us is this library user-friendly, maintainable and what improvements can we add to this.
There are a lot unnecessary inefficiency on that code. For big data, it makes a lot difference.
The current version, time complexity-wise... $$text{ mean() and mode() are }O(n)$$. $$text{range() and median() are } O(n .log(n))$$.
All could be implemented in average time complexity O(n). Not only that, checking if the list is ordered, taking the mean, max, min and mode could be done with one loop.
Once, you have checked the list is ordered, the median can be calculated as in this code. If is not, you can use "Median find algorithm": https://rcoh.me/posts/linear-time-median-finding/
Just as example, the code below shows how to detect ordering, and how to find the max, the min and the mean of a list. Now, you can implement "Median Finding" and add the mode calculation to have an efficient MMMR library.
const medianFromSorted = (numbers) => {
const middle = Math.floor(numbers.length / 2);
if (numbers.length % 2 === 0) {
return (numbers[middle - 1] + numbers[middle]) / 2;
}
return numbers[middle];
};
const mmr = (numbers) => {
if (numbers.length === 0) {
return {mean: undefined, moda: undefined, median: undefined, range: undefined};
}
if (numbers.length === 1) {
return {mean: numbers[0], moda: numbers[0], median: numbers[0], range: [numbers[0], numbers[0]]};
}
let total = 0, i;
let asc = numbers[0] <= numbers[1];
let desc = numbers[0] >= numbers[1];
let min = numbers[0];
let max = numbers[0];
let previous = numbers[0];
for (i = 0; i < numbers.length; i++) {
total += numbers[i];
if (numbers[i] < min)
min = numbers[i];
if (numbers[i] > max)
max = numbers[i];
asc = asc && (previous <= numbers[i]);
desc = desc && (previous >= numbers[i]);
previous = numbers[i];
}
let median;
if (asc || desc)
median = medianFromSorted(numbers);
else
median = medianFinding(numbers);
return {
mean: total / numbers.length,
median: median,
range: [min, max]
}
};
```
Answered by rdllopes on November 17, 2021
Rotara already mentioned some great points - the comments can definitely be improved and follow common conventions, sorting should happen on copies of the data instead of the original data, and the input should be validated. Consider the case of mean
when an empty array is passed in. The value of numbers.length
will be 0
so this leads to division by zero, which results in NaN
. Perhaps it would be wise to throw
an exception in that case.
Let's look at the mean()
function. It has a traditional for
loop, though the iterator variable, i.e. i
, is only used to dereference values in the array. That could be changed to the simpler syntax of a for...of
loop
const mean = (numbers) => {
let total = 0;
for (const num of numbers) {
total += num;
}
return total / numbers.length;
};
Moreover, a functional approach could be used with Array.reduce()
:
const sum = (a, b) => a + b
const mean = numbers => {
return numbers.reduce(sum, 0) / numbers.length;
}
I tried these various functions with an array of five elements, as well as fifteen elements. It seems the original code is somewhat faster than the other approaches in FF and Chrome. So for a library that may be used with many projects it may make sense to keep with the original syntax. While I don't condone plagiarism I did peek at the source of mathjs and saw that its mean()
uses a custom function deepForEach()
which is basically a traditional for
loop. As this article explains, the for...of
loop uses an iterator method for each iteration and that is the cause of the slower execution.
As this freecodecamp article explains:
“The first step in optimizing the amount of work in a loop is to minimize the number of object members and array item lookups.
You can also increase the performance of loops by reversing their order. In JavaScript, reversing a loop does result in a small performance improvement for loops, provided that you eliminate extra operations as a result.”
This means two optimizations for those loops would be to do one of the following:
store length of the array in a separate variable instead of comparing the property each time
for (let i = 0, const l = numbers.length; i < l; i += 1) {
add Items in reverse to eliminate steps
// minimizing property lookups and reversing
for (let i = numbers.length; i--; ){
This could also be written as a while
loop
Notice let i
can be moved inside the for
loop since the scope can be limited to just that block.
I also compared the mode
function with the mathjs implementation for the same function. It appears that implementation uses a mapping of values to counts, which would require more memory. It also returns an array of all values that have the most number of occurrences. The MMMRjs implementation appears to return the first value that contains the maximum number of occurrences. This should be documented in the comments.
Beyond that I don't see much to suggest, except maybe unit tests to cover all scenarios that should be handled by the code. The code looks decently written, especially using const
and let
where appropriate. It also has consistent indentation of nesting levels.
You mentioned "Most of the points mentioned in Ratora's answer were already implemented please check the updated source code by downloading the library from the link given above". I noticed that range
was updated to call slice()
on the input array before calling sort
. However the returned array is not stored or used in the return
statement. Thus the output may be incorrect if the values aren't already sorted. See the snippet below for a demonstration.
const range = (numbers) => {
numbers.slice().sort();
return [numbers[0], numbers[numbers.length - 1]];
};
const nums = Object.freeze([0.4, 0.2, 0.3]);
const expected = [0.2, 0.4];
const outputRange = range(nums);
console.log('output from range(): ', outputRange);
console.log('expected output: ', expected);
Answered by Sᴀᴍ Onᴇᴌᴀ on November 17, 2021
The comments such a This is Mean.
are pointless.
And the comment
// To call/run this function we can type mean(Your array of numbers separated with commas ',');
is a bit strange. Are they targeting people who don't know basic JavaScript syntax? Then why mention commas, but not the brackets ([...]
) needed for an array.
It probably would be a good idea to instead use a standard comment format for documentation such as jsdoc.
The comparison function used for sorting ((a, b) => a - b
) is pointless, because that is the default.
In median
you are creating a copy of the input array with .slice()
before sorting it, so that it isn't modified, which is a good thing. You should do the same in mode
and range
.
You should consider validating the input. mean
, mode
and range
break or give unexpected results if you give them an empty array. It's probably best to check if they are receiving an array at all.
Answered by RoToRa on November 17, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP