search

kchapelier / node-mathp

Math utility for node
MIT License
9 stars 1 forks source link

Loop consistency/terseness refactor #12

Closed megawac closed 9 years ago

megawac commented 9 years ago

The goal of this is to make the loops terser and more consistent in general. I have to go but let me know if you disagree

kchapelier commented 9 years ago

Thanks for the PR.

I'll run some benchmarks and merge it in later today.

kchapelier commented 9 years ago

Here is a benchmark for the browsers : http://jsperf.com/mathp-pr12

I have yet to test in node and io.js, but so far I'm in favor of only merging the rms and euclideanDistanceN.

Waiting for your input.

megawac commented 9 years ago

The changeset of the code here is extremely minimal from a JS engines perspective (pretty much just changing the order of if statements); as such benchmarking this changeset isn't particularly feasible (especially with jsperf) - the actual speed should be ~~ equivalent. See http://mrale.ph/blog/2012/12/15/microbenchmarks-fairy-tale.html if you're interested

The justifcation for this change is more for simplicity/terseness throughout the codebase (I tried to be consistent for each function), leading to more readable code (IMO) and a smaller output size.

On Sun, Mar 8, 2015 at 7:17 AM, Kevin Chapelier notifications@github.com wrote:

Here is a benchmark for the browsers : http://jsperf.com/mathp-pr12

  • For Firefox and Safari the only thing to note is that the new euclideanDistanceN is slightly faster.
  • The old minkowskiDistanceN performs better on both Chrome and Canary.
  • The new euclideanDistanceN performs worse on Chrome but better on Canary.
  • The new rms performs better on Canary
  • The rest of the performances on Chrome and Canary is slightly skewed toward the old implementations, but it's mostly noise.

I have yet to test in node and io.js, but so far I'm in favor of only merging the rms and euclideanDistanceN.

Waiting for your input.

— Reply to this email directly or view it on GitHub https://github.com/kchapelier/node-mathp/pull/12#issuecomment-77743981.

kchapelier commented 9 years ago

I'm aware of the issues of the microbenchmarks but it is, afaik, the only tool at our disposal to evaluate the potential performance issues. Obviously, in our case we are mostly measuring how the current JS engines optimize different flavors of the same methods. It is nonetheless an interesting metrics.

As for readability, I'm usually in favor of the "one return statement per function" rule. IMHO it usually makes it easier to reason about and maintain the code, though those advantages are probably be more apparent in more complex codebases.

megawac commented 9 years ago

The decision is ofc up to you, I find the returning immediately with the result is known to be more clear personally (and leads to less nesting). Besides theres multiple returns in several of the other functions already

kchapelier commented 9 years ago

Merged it all but the minkowskiDistanceN (reverted it to its previous implementation).