1
\$\begingroup\$

Just wondering if you guys think this is a good way to shuffle a deck or a bad way. Updated to show the GetRandom function as requested.

Private Sub DeckShuffle()
        ' shuffles the deck 4 times.
        For shufflingCards As Integer = 0 To 3
            ' goes through each card and changes position once each time it goes through shufflingCards
            For shuffleEachCard As Integer = 0 To 51
                ' gets a random number 0 - 51
                Dim placeHolder As Integer = GetRandom(0, 51)
                ' creates a temporary card for swapping cards
                Dim tempCard As Card = deck(placeHolder)
                ' makes the random number card now the card we were shuffling
                deck(placeHolder) = deck(shuffleEachCard)
                ' replaces the card we were shuffling with the one we were saving as a temporary
                deck(shuffleEachCard) = tempCard
            Next
        Next
    End Sub

' Gets a random number for shuffling.
Private Function GetRandom(ByVal Min As Integer, ByVal Max As Integer) As Integer
    Static Generator As System.Random = New System.Random()
    Return Generator.Next(Min, Max)
End Function
\$\endgroup\$
  • \$\begingroup\$ You only need one pass through the deck. \$\endgroup\$ – teppic Nov 3 '17 at 0:07
1
\$\begingroup\$

You won't need to shuffle the deck 4 times. For a use case of shuffling a deck of cards using the System.Random once should be sufficient.

But there is still something to say about the GetRandom() method:

  • based on the .NET naming Guidelines method arguments should be named using camelCase casing. Although VB.NET is case-insensitive you should improve your coding style by applying the guidelines because other developers will expect it.
  • the declaration and initialisation of Static Generator As System.Random = New System.Random() could be simplified like Static generator As New System.Random() which takes into account naming local variables using camelCase casing as well.

DeckShuffle()

  • If this method is part of a Deck class you should just name it Shuffle otherwise ShuffleDeck would be better because methods should be named using verbs or verb phrases.

  • The inner loop variable shuffleEachCard isn't named good as well. currentCard would be more significant (you can't shuffle a single card).

  • Dim placeHolder As Integer = GetRandom(0, 51) will get you a random number in the range of 0..50 because the maxValue parameter of Random.Next is exclusive. Hence you should change it to GetRandom(0, 52) to get a random number in the range 0..51. In addition randomCardNumber would be a better name.

\$\endgroup\$
  • \$\begingroup\$ Wow, thank you for answering my review. I thought this thread was d.a.d. I'll implement the changes like you have suggested :) \$\endgroup\$ – Brandon Nov 3 '17 at 10:15
2
\$\begingroup\$

Another way to shuffle a deck is to start of with your full deck (oldDeck) and an empty deck (newDeck). This uses the same random generator function in the OP. I haven't tried to properly code this because the algorithm is the important message.

While size of oldDeck > 0
  chosenCard = getrandom(0,size of oldDeck-1)
  remove chosenCard from oldDeck ' reduces the size of oldDeck by 1
  append chosenCard to newDeck
'Wend
return newDeck ' or reassign newDeck to oldDeck, depending on the broader context

This algorithm would be particularly useful if a class of Deck existed, and it could be a member function as noted in the previous reply. You could then consider having a class of Card which includes a suit and value. A Deck would then be a collection of Cards, and the built-in collection functions become your friends. Of course, this amount of effort depends on what you are going to do with your Deck.

\$\endgroup\$
1
\$\begingroup\$

@Brandon, you only need one pass through the deck.

A question: why don't you try the Fisher-Yates shuffle?

Pseudocode from Wikipedia article:

-- To shuffle an array a of n elements (indices 0..n-1):
for i from 0 to n−2 do
     j ← random integer such that i ≤ j < n
     exchange a[i] and a[j]

Your code would look like this:

        For shuffleEachCard As Integer = 0 To 50
            ' gets a random number 
            Dim placeHolder As Integer = GetRandom(i, 52)
            ' creates a temporary card for swapping cards
            Dim tempCard As Card = deck(placeHolder)
            ' makes the random number card now the card we were shuffling
            deck(placeHolder) = deck(shuffleEachCard)
            ' replaces the card we were shuffling with the one we were saving as a temporary
            deck(shuffleEachCard) = tempCard
        Next
\$\endgroup\$

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.