current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'd like to practice oojs so I've written code:

function Randoms(c, l, h) {
    var count = c,
        min = l,
        max = h,
        nums = Array(),
        self = this;

    //returns random number from given max and min
    var getRand = function () {
        return Math.floor(Math.random() * (max - min) + min);

    };

    //checks if given number is unique
    var check = function (a) {
        for (var i = nums.length - 1; i >= 0; i--) {
            if (nums[i] == a) {
                return true;
            }
        }
        return false;
    };

    //makes array with random, unique numbers
    this.build = function () {

        var r = getRand();

        if (nums.length === 0) {
            nums.push(r);
        } else {
            do {
                r = getRand();
            } while (check(r));

            nums.push(r);
        }

        if (nums.length == count) {
            return true;
        } else {
            this.build();
        }
    };

    //returns array
    this.get = function () {
        return nums;
    };

    //simulating constructor
    this.build();

}

var a = new Randoms(1, 1, 18);
console.log(a.get());

The purpose is to get array with random, unique numbers.

What do you think? Is it a good code or does it require modyfications?

share|improve this question

1 Answer 1

up vote 3 down vote
  1. Your check method is inefficient for large datasets. I'd recommend using a hash instead. This will eliminate the need of iterating over the array every time you want to check if a number is there.
  2. It is possible for your code to enter an infinite loop if count is greater than max - min. You need to test for this.
  3. The code can be simplified a bit as well.
  4. The recursion slows down the generation dramatically.

I've take then liberty of performing some optimizations: http://jsfiddle.net/FMSpG/2/

function Randoms(c, l, h) {
    var count = c,
        min = l,
        max = h,
        nums = {},
        out = [],
        r, len = 0;

    if (max - min < count) count = max - min;

    var getRand = function() {
        return Math.floor(Math.random() * (max - min) + min);
    },  check = function(a) {
        return nums[a];
    },  add = function(a) {
        nums[a] = 1;
        out.push(a);
        len++;
    };

    (function init() {
        while (len < count){
            if (len == 0) r = getRand();
            else while (check(r = getRand()));
            add(r);
        }
    })();

    this.get = function() {
        return out;
    };
}

And here's a jsPerf showing the huge difference in performance for large datasets:

http://jsperf.com/unique-randomization

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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