Number gen NaN

Hey,

I've tried Googling this and implementing solutions, but it was to no avail. For some reason my gen class is coming up NaN.

gen class

function cGenerator()
{

this.upperRange;
this.lowerRange;
this.rolledNumber;
// set the lower and upper ranger
this.set_range = function(lowerSet, upperSet)
{
   this.lowerSet = parseInt(lowerSet);
   this.upperRange = parseInt(upperSet);
  
};
// roll a number between lower and upper range set with set_range.
this.roll = function(min, max) {

    this.rolledNumber = parseInt(Math.floor(Math.random() * (parseInt(this.upperRange) -  parseInt(this.lowerRange) + 1)) + parseInt(this.lowerRange));
    return parseInt(this.rolledNumber);

};

}

Code (JavaScript):

implementation

 gen.set_range(1, 5);
            var roll = gen.roll();
             alert(roll);

Code (JavaScript):

 

You're using "lowerSet" where you should have used lowerRange (the assignment)

 

Both of the above are correct -- and I think you got a bit too complicated for your own good on that -- also if you are going to use a constructor function, you might as well set the range while at it. This works, I left the setRange function in there should you want to change the output range on the fly.

function cGenerator(min, max) {

	this.min = parseInt(min);
	this.max = parseInt(max);
	this.range = this.max - this.min;
	this.lastValue = 0;
		
	this.setRange = function(min, max) {
		this.min = parseInt(min);
		this.max = parseInt(max);
		this.range = this.max - this.min;
	}
	
	this.roll = function() {
		return this.lastValue = this.min + Math.floor(Math.random() * this.range);
	}
	
} // cGenerator()

var	gen = new cGenerator(1,5);
alert(gen.roll());

Code (markup):

You'll notice I typecast to integer early, and pre-calculate "range". It's usually more optimal to handle it that way since it's far more likely that "roll" will be called more often than setRange and/or the constructor.

 

Deathshadow's solution seems much more elegant.
However, I wonder why did you repeat the code in lines 3-5 and 9-11; wouldn't it be more appropriate to call a method
this.setRange(min, max) instead of lines 3-5?

Furthermore, would it be better to make it a factory instead of constructor (I'm not sure if I use right terms, what I mean is:

function cGenerator(min, max) {
  return {
    min : parseInt(min),
    max : parseInt(max),
       //...
   }
} // cGenerator()

Code (markup):

In both cases, which is more savvy, to keep some variables local, such as lastValue? I know the local variables are "trapped" inside object and never disposed, so it can waste memory, but what about public variables (this.lastValue)? Which is better?

Also, final question: In particular case, is it better to create new instance every time

var gen = new cGenerator(1,5);
result = gen.roll();
var gen2 = new cGenerator(1,6);
result = gen2.roll();
var gen3 = new cGenerator(11,99);
result = gen3.roll();

Code (markup):

instead of just updating the values by calling setRange method before rolling each time:

var gen = new cGenerator();
gen.setRange(1,5);
result = gen.roll();
gen.setRange(1,6);
result = gen.roll();
gen.setRange(11,99);
result = gen.roll();

Code (markup):

 

Calling setRange would be less code, but it would run slower on startup due to the overhead of calling the method. Older versions of IE may also interpret setrange's "this" as referring to the method's "this" instead of the parent object's "this" if you don't define it first.... which is really ***tarded, but did we mention it's an IE problem?

Generating a new object every blasted time will (or at least should) trigger garbage collection AND memory allocation, making it WAY slower and making memory thrash if you use the same variable. If you use a different variable like your example, you are just making more and more and more and more copies sucking on RAM like it's candy. Unless you need to preserve the result, there's no real reason to do that UNLESS you are trying to save run time by predeclaring multiple ranges and then only calling the .roll method over and over.

var
	d6 = new cGenerator(1,6),
	d10 = new cGenerator(1,10),
	d6p3 = new cGenerator(4,9);
	
for (var t = 0; t < 100; t++) {
	log(
		d6.roll() + ' : ' +
		d10.roll() + ' : ' +
		d6.roll() + ' : ' +
		d6p3.roll()
	);
}

Code (markup):

THAT is a usage scenario where creating multiple instances of the object would be faster, as then you aren't creating new copies of the object and it's methods and setting up all those values inside the loop; you're just calling the methods.

Making it functional instead of an object would mean that ALL that math is run EVERY time you call it, and that parameters are passed EVERY time you call it. That's just more overhead and slower if you are going to .roll() more than once. Now, if you are only going to .roll() ONCE for each range EVER, then yes, procedural would be faster with less overhead. If you are going to call .roll() more than once for each set of ranges, the object is faster.

That's an aspect that the "functional vs. object" people on BOTH sides usually miss; the usage scenario would actually determine which method of doing it is best, NOT the method itself. That's why there are so many different ways of doing things.

 

Thank you

 

Hey,

Thanks guys, I actually caught the variable definition error. Thanks for the simplified code too, will soak it in

 

What about this approach:

var cGenerator = {};

cGenerator.getRange= function(min,max) {
    this.min = parseInt(min);
    this.max = parseInt(max);
    return this.max - this.min;
};
   
cGenerator.roll = function(min,max) {
    return this.lastValue = this.min + Math.floor(Math.random() * this.getRange(min,max));
};

alert(cGenerator.roll(1,5));

Code (markup):