1.01 KB,
patch

Robert Ginda
:
review+

Details  Diff  Splinter Review 
Noticed the following issues in utils.js, and think I have alternatives which will save some cycles and possibly improve readability. 1) Unneeded use of parseInt() to find integer part of an expression. Fix: replace each parseInt() with Math.floor() globally. 2) Suggest the following replacements: function getRandomElement (ary) { return ary[Math.floor (Math.random() * ary.length)]; } function roundTo (num, prec) { var tp = Math.pow (10, prec); return Math.floor (Math.round(num * tp)) / tp; } 3) Unsure how randomRange() is supposed to work. It really depends on whether we are assuming any/all integer inputs, and if the output should be integer. My guess is that what is wanted is a random integer between integers min and max, inclusive. This would be: function randomRange (min, max) { if (typeof min == "undefined") min = 0; if (typeof max == "undefined") max = 1; return min + Math.floor(Math.random() * (max  min + 1)); }
Comment 1•16 years ago


Created attachment 67223 [details] [diff] [review] some small adjustments to utils.js
Updated•16 years ago

(Assignee)  
Comment 2•16 years ago


function getRandomElement (ary) {  var i = parseInt (Math.random() * ary.length)  if (i == ary.length) i = 0;  return ary[i]; + return ary[Math.floor(Math.random() * ary.length)]; } There is no element ary[ary.length], which is what that wacky test was looking out for. Perhaps return ary[Math.floor(Math.random() * (ary.length  1))]; The other two hunks have extra parens around the first Math.round(). fis that and r=rginda.
Math.random() returns numbers < 1, so the array would never get indexed at ary.length. To illustrate, assume ary.length == 5, and run this as many times as you want: alert(Math.floor(Math.random() * 5)); You'll never generate a 5. Changing to length  1 would mean that the last element would never be returned. Again, with ary.length == 5, we would have Math.random() * 4, which would only generate 0, 1, 2, and 3, but never a 4, even though that's a valid index.
Comment 4•16 years ago


ok, from the spec: "[random()] Returns a number value with positive sign, greater than or equal to 0 but less than 1"
Comment 5•16 years ago


Created attachment 69007 [details] [diff] [review] new patch, missed randomRange
Comment 6•16 years ago


Created attachment 69008 [details] [diff] [review] another try a little too quick on the submitting
(Assignee)  
Comment 7•16 years ago


Comment on attachment 69008 [details] [diff] [review] another try r=rginda
Comment 8•16 years ago


checked in
Updated•13 years ago

Description
•