HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Generating random numbers and decks of cards

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomcardsnumbersgeneratinganddecks

Problem

So I have some code here:
First a simple random # generator and a array-choosing function:

function Rand(min,max) 
 {
 return parseFloat(Math.floor(Math.random()* max-min+1)))+parseFloat(min);
 }

 function Choose(arr)
 {
//Returns an element from an array at random.
return arr[Math.floor(Math.random()*arr.length)];
 }


and second a Card shuffler:

function CardDeck() {
var Cd=["Ace","2","3","4","5","6","7","8","9","10","Jack","Queen","King"];
var H=[];
var S=[];
var D=[];
var C=[];
var Result=[];
var Dk=document.getElementById("Deck Count").value;
for(i=0;i<Cd.length;i++) {
    S[i]=Cd[i]+" of Spades";
    H[i]=Cd[i]+" of Hearts";
    C[i]=Cd[i]+" of Clubs"; 
    D[i]=Cd[i]+" of Diamonds";}
if (Dk=="4") {P=S.concat(C,D,H);}
else if (Dk=="5") {
    var St=[];
    for(i=0;i<Cd.length;i++) {St[i]=Cd[i]+" of Stars";}
    P=S.concat(C,D,H,St);}
else if (Dk=="6") {
    var Rk=[];
    var Wh=[];
    for(i=0;i<Cd.length;i++) {
        Rk[i]=Cd[i]+" of Rackets";
        Wh[i]=Cd[i]+" of Wheels";}
    P=S.concat(C,D,H,Rk,Wh);}
for(i=0;i<Dk*Cd.length;i++) {
    var Q=Choose(P);
    R=P.indexOf(Q);
    Result[i]=(i+1)+": "+Q;
    P=P.slice(0,R).concat(P.slice(R+1));}
document.getElementById("Cards").innerHTML=Result.join("\n");}


Is there an easy way to make this faster or at least declare the arrays faster instead of just doing a=[],b=[]...

Additionally, is there a way to get a better RNG with a longer period in base JS (no libraries, as this was built on base JS)

Solution

First off, your code is borderline unreadable. Use proper indentation. Add whitespace (horizontal and vertical). Follow conventional guidelines (lowercase camelCase names for most things). And give your variables actual names - bytes are cheap.

If you want really compact code, use a minifier - after writing the code in readable, sane manner. This looks like a text dump of a fiendish Excel spreadsheet or something.

For the few things that I could read, and which stand out:

-
You Rand [sic] function is doing a bunch of unnecessary stuff. You parseFloat something that's guaranteed to be an integer, because you called Math.floor() on it. You parseFloat on min when adding it at the end, but you don't do it when you subtract it from max.

And it's broken (which makes it technically off-topic for CodeReview, by the way).

Firstly, it doesn't even run because you've got one close-parenthesis too many, so there's a syntax error.

Secondly, you're not paying attention to operator precedence: Multiplication precedes addition and subtract. So your code is saying Math.random() * max then subtracting min and finally adding 1. Hence, if you call Rand(9, 10), you're liable to get any number from 1-10, rather than a number from 9-10.

Just say:

function rand(min, max) {
  var span = max - min;
  return Math.round(Math.random() * span) + min;
}


-
As for Choose [sic], you can use the bitwise-floor trick:

function choose(arr) {
  return arr[Math.random() * arr.length | 0];
}


-
When appending to and array, use push. Don't just assign to an index. E.g.

spades.push(values[i] + " of Spades");
hearts.push(values[i] + " of Hearts");
clubs.push(values[i] + " of Clubs");
diamonds.push(values[i] + " of Diamonds");


As for "better RNG", that's a StackOverflow question if anything. JavaScript has Math.random() and that's it.

Code Snippets

function rand(min, max) {
  var span = max - min;
  return Math.round(Math.random() * span) + min;
}
function choose(arr) {
  return arr[Math.random() * arr.length | 0];
}
spades.push(values[i] + " of Spades");
hearts.push(values[i] + " of Hearts");
clubs.push(values[i] + " of Clubs");
diamonds.push(values[i] + " of Diamonds");

Context

StackExchange Code Review Q#136602, answer score: 5

Revisions (0)

No revisions yet.