Can you review for user prompted generator?

Question

Is there any way I could have made this cleaner/better? Thanks

function myRandom(bottomNum,topNum) {
  return Math.floor(Math.random() * (topNum - bottomNum)) + bottomNum;
}

var topNum = 0;
var bottomNum = 0;

var minNum = prompt("You will choose two numbers and a random number will be generated between them. Your first number is:");

var maxNum = prompt("Thank you. You chose:" + minNum + ". What is your second number?");

alert("Thank you. You have chosen " + minNum + " and " + maxNum + " as your two numbers. Next there will be a random number generated between those two.");

minNum = parseInt(minNum);
maxNum = parseInt(maxNum);

if (minNum > maxNum) {
  topNum = minNum;
  bottomNum = maxNum;
} else {
  topNum = maxNum;
  bottomNum = minNum;
}      

document.write(myRandom(topNum,bottomNum));

Answer 1 · 2016-01-06T19:37:37Z

January 6, 2016 7:37pm

Hi John,

I have some suggestions on shortening the code down but first I wanted to point out that you've reversed the order of topNum and bottomNum when calling your function. In your function definition you're expecting the lower number to be first but when you call it you're passing the higher number in first. This effectively reverses the 2 inside the function.

It still appears to work ok but there are some differences at the ends of the range as to what you can get out of it. I can explain that in more detail if you're interested. Let me know.

Here's some suggestions for shortening down the code:

Since you're not asking the user to enter the low number first I would not make the distinction between min and max at that point in the program. minNum isn't necessarily going to be storing the min number at that point so it might be more appropriate to use general names like num1 and num2.

Also, you can pass the result of prompt into the parseInt method allowing you to eliminate the parseInt lines. The string concatenation you're doing still works out because the numbers will be type-casted to strings for the concatenation and still be numbers afterwards.

I would eliminate the topNum and bottomNum variable and move the document.write into the if/else block in order to simplify that part of the code.

Here's some code with comments to show you what I mean:

function myRandom(bottomNum,topNum) { // Note: lower number should be passed in as first argument
  return Math.floor(Math.random() * (topNum - bottomNum)) + bottomNum;
}

// pass the result of prompt into the parseInt method and switched to more general variable names
var num1 = parseInt(prompt("You will choose two numbers and a random number will be generated between them. Your first number is:"));

var num2 = parseInt(prompt("Thank you. You chose:" + num1 + ". What is your second number?"));

alert("Thank you. You have chosen " + num1 + " and " + num2 + " as your two numbers. Next there will be a random number generated between those two.");

if (num1 > num2) {
  document.write(myRandom(num2,num1)); // num1 is bigger so pass that in as second argument
} else {
  document.write(myRandom(num1,num2)); // num1 wasn't bigger so it should be the 1st argument
}

This isn't necessarily a better way to do it but it could be a different way to think about it.

Answer 2 · 2016-01-06T19:49:55Z

January 6, 2016 7:49pm

Thanks Jason, I appreciate the time and effort to review. I definitely wouldn't have thought of combining the document.write(); into the if else statement. I guess my brain just doesn't think that way. :)

You also commented on how I had the two numbers reversed and how that could effect the range. I'd love to know more about that if you have the time/desire to discuss it. thanks

Welcome to the Treehouse Community

Looking to learn something new?

John Maier

John Maier

Can you review for user prompted generator?

2 Answers

Jason Anello

Jason Anello

John Maier

John Maier