fordth / jinqJs

jinqJs provides a simple way to perform SQL like queries on javaScript arrays, collections and web services using LINQ expressions.
Other
93 stars 29 forks source link

_min not handling values of zero properly #27

Open mcoakley opened 7 years ago

mcoakley commented 7 years ago

During building tests for coverage I updated and changed the data that is used by the tests. (More data coverage is needed but that's a different issue.) I've found that the _min function will not handle values that are exactly zero.

In the code...

if (!hasProperty(minValue, key)) { minValue[key] = 0; }

if (minValue[key] === 0 || rValue < minValue[key]) { 
  minValue[key] = rValue;
}

In the _min function, the test for minValue having the current key sets the key if it not found and sets it to zero (presumably as a flag) BUT if the real value of the data is a zero the next statement will effectively wipe out this potentially zero minimum value. I'm going to re-write this section as:

if (!hasProperty(minValue, key)) || rValue < minValue[key]) { 
  minValue[key] = rValue;
}

This will ensure that if no minimum value exists the first value retrieved is assigned and the full range of potential values is covered.

I'm fixing this in my fork of jinqJs which I should be finishing soon and then plan to submit a PR

mcoakley commented 7 years ago

I also found the same issue with _max. I will fix it the same way but generally speaking zero should not be the maximum value so this bug most likely wouldn't have been seen.

fordth commented 7 years ago

Great, sounds good. Im looking forward to your PR when its ready.

fordth commented 7 years ago

Im going to have to update the jinqjs.readme.io documentation :)

mcoakley commented 7 years ago

I think you should go over to my fork just to start to see where I've been going. Also, understand that I'm focusing on building the Node side and then using build tools to build a browser version (w/minification).

fordth commented 7 years ago

Yup, I already started doing just that