1
\$\begingroup\$

A few days ago, I've made a code to equally distribute a set of numbers and retrieve them randomly. Today, I've rewritten the code (most of it, at least). I've followed most of the advice and tried to keep it clean, but also as fast as possible. The new code has a few more comments than the last one (they were needed) and the code suffered a huge simplification.

( function ( window ) {

    window.RNG = function ( min, max ) {

        var nan = function ( val ) {
                /*
                 * Checks if the value is an usable number or NaN or Infinity
                 * When dividing by 1, it will convert the value to a number or NaN
                 * NaN values return false when compared with themselves ( NaN != NaN returns true)
                 *
                 * The 2nd part of the verification will check if the value is infinite
                 * Infinity will return Infinity if you sum or subtract 1
                 * E.g.: Infinity - 1 == Infinity --> returns true
                 */
                return ( ( val / 1 ) != ( val / 1 ) ) || ( val - 1 == val );
            },
            //will contain all the available numbers
            avail = [];

        if ( nan( min ) )
        {
            throw new TypeError( 'Expecting Number or NumericString, got ' + ( min != min ? 'NaN' : ( min - 1 == min ? 'Infinity' : typeof min) ) );
        }
        if ( arguments.length > 1 )
        {
            if ( nan( max ) )
            {
                throw new TypeError( 'Expecting Number or NumericString as the second parameter, got ' + typeof max );
            }
            /*
             * converts the numbers to integers ( n>>=0 -> n = n >> 0)
             * and check if the minimum is higher than the maximum,
             * throwing an exception if that is the case
             */
            else if ( ( min >>= 0 ) >= ( max >>= 0 ) )
            {
                throw new RangeError( 'The 2nd parameter should be higher than the first' );
            }
            else if ( ( max - min ) > 268435455 )
            {
                //big numbers crash hard, so we must stop them before
                throw new RangeError( 'The difference between ' + max + ' and ' + min + ' is higher than 268435455' );
            }
        }
        else
        {
            max = min >> 0;
            min = 0;
        }

        return {
            valueOf: function ( ) {

                if( !avail.length )
                {
                    for( var i = 0; i <= max - min; i++ )
                    {
                        avail[i] = min + i;
                    }
                }

                if( avail.length > 1 )
                {
                    //returns a random element between 0 and avail.length (excluded)
                    return avail.splice( ( window.Math.random() * avail.length ) >> 0, 1)[0];
                }
                else
                {
                    return avail.splice( 0, 1 )[0];
                }

            },
            toString: function ( ) {
                return this.valueOf() + '';
            }
        };

    };

} )( Function('return this')( ) );

To use it, simply create a new object:

var random_number = new RNG( 50 ); //same as 0,50

Then, you use this to generate numbers, with direct access:

alert( random_number ); //should alert a number between 0 and 50

This also works for negative numbers but it won't work for decimal numbers.

In terms of readability and performance, what else should I improve?

If you notice, on the answer to the question, one of the pieces of advice was to avoid using:

else if( ( min = min >> 0 ) >= ( max = max >> 0 ) )

I've completely ignored it to keep my code a little DRY and to improve performance.

\$\endgroup\$
1
\$\begingroup\$

Input validation

You shouldn't just change the user input if it is wrong. If a user calls var random_number = new RNG( 10, 19.9 ); they will expect to get number between 10 and 19.9. But that's not what they will get, because their assumption is wrong (your random number generator only generates integers). A user should be informed about this, instead of just silently correcting their wrong assumption.

Regarding I've completely ignored it to keep my code a little DRY and to improve performance: Are you sure that there is a performance problem? Because it doesn't seem like it (instantiating a new RNG is something that probably isn't done all that often), and the code is quite hard to read because it does too many things (changing data and comparing data), and it uses tricks (shifting instead of casting), which is generally not recommended, as it hurts readability.

Array Generation

This:

                for( var i = 0; i <= max - min; i++ )
                {
                    avail[i] = min + i;
                }

Would be a more natural and easier to read written as this (you can immediately see if min and max are inclusive or not:

                for( var i = min; i <= max; i++ )
                {
                    avail[i] = i;
                }

Comments

It's good that you added comments (although using a bit less tricky code would probably reduce the need for some comments in the first place).

But this comment is a bit misleading: //returns a random element between 0 and avail.length (excluded): The next line also changes the array, something the comment doesn't mention at all. I would rewrite it as removes and returns random array element (you don't need the range, as its from the complete array, which makes sense).

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. I know I'm guilty of changing the user input. What do you think I should do about that? Throw an exception? Regarding your rewrite, I agree with you, the for loop should be rewritten. But, you have this: avail[i] = i;, which should be avail[avail.length] = i; or avail.unshift(i);. About the part that I completely ignored... Well, I need to shift it to convert to int (or divide by 1), and I will have to store the same value on the variable. To do not repeat myself, I do both on a single line, since I have to also compare if the introduced values are correct. \$\endgroup\$ – Ismael Miguel Mar 4 '15 at 21:16
  • \$\begingroup\$ @IsmaelMiguel yes, I think I would throw an exception (it's invalid input after all). And your right, avail[i] would of course not work like that. DRY doesn't mean that you should put different functionality on the same line. having something like var min = Math.floor(min); var max = Math.floor(max); if (min >= max) { throw } else if (too big) { throw } would not violate DRY, but it is a lot more readable than if ( ( min >>= 0 ) >= ( max >>= 0 ) ) { throw } else if (too big) { throw }. \$\endgroup\$ – tim Mar 4 '15 at 21:33
  • \$\begingroup\$ Totally agree. I will see what I will do. Any more ideas to improve the code? I was thinking about filling the array and then shuffle all the numbers in it (to be a little more unpredictible). Right now, this has a flaw that allows to predict a little when the maximum and minimum values will be "rolled". \$\endgroup\$ – Ismael Miguel Mar 4 '15 at 21:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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