2
\$\begingroup\$

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)

\$\endgroup\$
  • \$\begingroup\$ To be honest, I'm not sure what your question is? - Compacting code and better Random numbers? or making your code faster? declaring arrays faster? get a better RNG with a longer period? . Please let me remind you this is code review and not Stack Overflow. I suggest you should migrate your question or better still modify your post. \$\endgroup\$ – Siobhan Aug 1 '16 at 22:54
  • \$\begingroup\$ Woops yeah let me migrate this question over there. \$\endgroup\$ – Malachi Wadas Aug 2 '16 at 0:27
  • \$\begingroup\$ Your question appears to be about CardDeck() and the Choose() function that it uses. What is Rand(min, max) for, and why is it part of this question? \$\endgroup\$ – 200_success Aug 2 '16 at 2:22
  • \$\begingroup\$ Don't use the global i variable, declare it locally. \$\endgroup\$ – Roland Illig Aug 5 '16 at 22:43
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Alright, I see your suggestions, but my code has correct indentation and it works correctly I just input it wrong when putting in the question. \$\endgroup\$ – Malachi Wadas Aug 2 '16 at 3:39
  • \$\begingroup\$ @MalachiWadas Gotcha - that happens. And yeah, the indentation itself is correct, but there's not enough of it, IMO. I.e. so if-else branches are on the a single line (kudos for still insisting on curly braces for such lines, by the way), others aren't. Pick one way and stick to it, rather than being inconsistent. Well, it's consistently compacted, but the computer doesn't care about that. The same code, written in a clear, readable manner with ample whitespace will execute exactly as fast. \$\endgroup\$ – Flambino Aug 2 '16 at 11:46
  • \$\begingroup\$ Agree with comments on readability. A developer working in the modern javascript world should not care about things like extra whitespace, short variable names, comments, etc. as that is what minification is for. Write your JS such that developers (including yourself at some point down the line) can read it and make sense of it. \$\endgroup\$ – Mike Brant Aug 2 '16 at 15:56
3
\$\begingroup\$

I will simply build on answer by @Flambino who gives strong advice on code formatting and some of the math concerns.

I would like to add a few more thoughts:

  • I would decouple display concerns (i.e. adding things such as " of Spades") from the base functionality of this class or provide class methods to get human-readable output. There is no reason to have this formatting concern be a part of your fundamental data model.
  • I would strongly think about using a more structured class approach to this, with prototype methods implemented for things such as shuffle, draw card, deal (i.e. draw multiple card hands), etc.
  • You might consider defining a card class. Right now a card is basically just a string. Fundamentally a card object would have two properties - suit and value, but you could expand the card class to define things such as an appropriate image for the card, or to provide methods for human-readable formatting (i.e. " of Spades"). These really are not properties of a card deck, but rather the cards themselves, so to me this calls out for a separate class from card deck.
  • It seems odd to me to have a key piece of input (in this case Dk) be read from DOM. You have now tightly coupled your class to the display. Why not pass this in as input? I have similar concern over how to output into DOM. Why should this be hardcoded into the card deck definition? If you had a proper card deck class, you could simply interact with the class methods and have all your DOM updates done based on the values the class returns.
  • I don't know if I am simply missing it due to code readability concerns, but I am not understanding how you persist the state of the card deck. When a card is drawn, how do you remove it from list of cards available for the next draw?
\$\endgroup\$
  • \$\begingroup\$ The bottom four lines in the code will give you an idea as to what is going on here. Additionally, I'm not using classes because I'm just messing around with a bunch of random generators I'm making, its just for fun so I'm not really doing extremely fine-tuned coding \$\endgroup\$ – Malachi Wadas Aug 6 '16 at 21:18
  • \$\begingroup\$ @MalachiWadas Even if just for fun, I am guessing you asked for a code review to get a better understanding as to how you might improve the code you write. You don't necessarily need to apply them to your current work, though you think about these sorts of things when writing code that you need to be more production quality. \$\endgroup\$ – Mike Brant Aug 8 '16 at 16:44
  • \$\begingroup\$ @Mike Brant How do you code the persist the state of the card deck? I would like to create a game where the card is removed from the deck until all cards are exhausted. \$\endgroup\$ – user9249467 Aug 20 '18 at 19:50
  • \$\begingroup\$ @user9249467, please be aware that this is a two year old post and you may not get an immediate response. \$\endgroup\$ – Malachi Aug 20 '18 at 20:23
  • \$\begingroup\$ @user9249467 I am not intent of your comment. I had asked the question about persisting the deck in my original review, because it was not clear how that happens. For me, if I am going to be building a means to store a deck of cards in memory, I would guess an array data structure to hold card objects would be a good start, but as mentioned in the review, I would be likely to wrap a class around the basic storage array to provide "deck-type" functionalities like shuffle, deal, etc. \$\endgroup\$ – Mike Brant Aug 23 '18 at 21:42

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.