logic clean up in utils.js

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
minor
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: xyzzy, Assigned: Robert Ginda)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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
Status: NEW → ASSIGNED
Keywords: patch
(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.
(Reporter)

Comment 3

16 years ago
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
Attachment #67223 - Attachment is obsolete: true

Comment 6

16 years ago
Created attachment 69008 [details] [diff] [review]
another try

a little too quick on the submitting
Attachment #69007 - Attachment is obsolete: true
(Assignee)

Comment 7

16 years ago
Comment on attachment 69008 [details] [diff] [review]
another try

r=rginda
Attachment #69008 - Flags: review+

Comment 8

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Product: Core → Other Applications
You need to log in before you can comment on or make changes to this bug.