13
\$\begingroup\$

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.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ From the link in my last comment: "What should I not do? Do not change the code in the question after receiving an answer. Incorporating advice from an answer into the question violates the question-and-answer nature of this site.". I encourage you to read the section titled I improved my code based on the reviews. What next? especially option #1 \$\endgroup\$ Jun 4, 2020 at 16:07
  • \$\begingroup\$ @NANO You are welcome If you like to please drop an answer and tell us what can be done to make MMMR.js better. \$\endgroup\$ Jun 13, 2020 at 11:53

3 Answers 3

8
+350
\$\begingroup\$

Comments

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.

Sorting

The comparison function used for sorting ((a, b) => a - b) is pointless, because that is the default.

Mutating the input array

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.

Validating the input

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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your feedback we will implement the changes ASAP. \$\endgroup\$ Jun 4, 2020 at 14:27
  • 1
    \$\begingroup\$ Your changes were made except for the input validation because It is not necessary this library provides a basic structure for MMMR which programmers can use in Javascript without manually programming it. \$\endgroup\$ Jun 4, 2020 at 15:04
  • 2
    \$\begingroup\$ If you don't validate the input, then clearly document that and the expected input. \$\endgroup\$
    – RoToRa
    Jun 5, 2020 at 8:24
  • 1
    \$\begingroup\$ Yes thank you we will. \$\endgroup\$ Jun 5, 2020 at 13:08
6
+200
\$\begingroup\$

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.


Addition June 10, 2020

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);

\$\endgroup\$
6
  • \$\begingroup\$ Yes, Sam, you are correct we will try to make some changes ASAP. 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. \$\endgroup\$ Jun 8, 2020 at 10:30
  • \$\begingroup\$ If you would like the updated code reviewed then please consider posting a follow-up question \$\endgroup\$ Jun 8, 2020 at 15:29
  • \$\begingroup\$ @SamOnela thank you for telling but we don't want it to be reviewed I was just telling that the fixes told by Rotora were done. \$\endgroup\$ Jun 8, 2020 at 21:04
  • \$\begingroup\$ Okay I have added another section that covers the updated range() function \$\endgroup\$ Jun 10, 2020 at 13:31
  • \$\begingroup\$ Thank you for the updated answer, Sam, we will do the changes asap \$\endgroup\$ Jun 10, 2020 at 15:29
2
+200
\$\begingroup\$

Not an O(n) MMMR library

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]
  }

};
```
\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much your feedback and suggestions were helpful and we will see what we can do. \$\endgroup\$ Jun 12, 2020 at 8:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.