# Deck Shuffling Method

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

• You only need one pass through the deck. – teppic Nov 3 '17 at 0:07

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.

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

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.

@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