Dealing Poker Hands

I thought I'd give a shot at creating my own version of dealing 5-card hands to n players in VBA, printing them to columns and coloring hearts and diamonds red.

I felt I might have been a little repetitive and I had to jump through some hoops to avoid ByRef. Anyhow, what can I improve?

```
Option Explicit

Public Sub DealCards()
'Just dealing to sheet2
Sheet2.Range("A:Z").Clear
Dim numberOfPlayers As Long
numberOfPlayers = GetPlayers
If numberOfPlayers = 0 Then Exit Sub
Dim i As Long
Dim myPlayers As Variant

ReDim myPlayers(1 To numberOfPlayers, 1 To 6)

myPlayers = DealDeck(numberOfPlayers)
Sheet2.Range(Cells(1, 1), Cells(6, numberOfPlayers)) = Application.WorksheetFunction.Transpose(myPlayers)

Colorize numberOfPlayers

End Sub

Private Function GetPlayers() As Long
Dim result As Long
result = Application.InputBox("How many players?", "Number of Players", 2, Type:=1)
If result > 9 Or result = 0 Then
MsgBox "There aren't enough chairs or players for this game!"
GetPlayers = 0
Exit Function
End If
GetPlayers = result
End Function

Private Function DealDeck(ByVal numberOfPlayers As Long) As Variant
Dim dealHands As Variant
ReDim dealHands(1 To numberOfPlayers, 1 To 6)
Dim i As Long
For i = 1 To numberOfPlayers
dealHands(i, 1) = "Player" & i
Next

Dim myDeck(1 To 52) As Variant
Dim hand As Long
Dim card As Long
Dim handPosition As Long
For hand = 1 To numberOfPlayers
For handPosition = 2 To 6
TryAgain:
card = Int(52 * Rnd + 1)
If IsEmpty(myDeck(card)) Then
myDeck(card) = dealHands(hand, 1)
dealHands(hand, handPosition) = ConvertCards(card)
Else: GoTo TryAgain
End If
Next handPosition
Next hand
DealDeck = dealHands

End Function

Private Function ConvertCards(ByVal card As Long) As String
Dim club As Stri

Your code assumes Sheet2 is active, and throws run-time error 1004 on this line if that's not the case:

Sheet2.Range(Cells(1, 1), Cells(6, numberOfPlayers)) = Application.WorksheetFunction.Transpose(myPlayers)

As the latest Rubberduck build could have told you, Cells implicitly references ActiveSheet:


Implicit references to the active sheet make the code frail and harder to debug. Consider making these references explicit when they're intended, and prefer working off object references.

Here are the rest of the relevant inspection results:

Warning: Member 'Cells' implicitly references ActiveSheet - (Book2) VBAProject.Module1, line 15
Warning: Member 'Cells' implicitly references ActiveSheet - (Book2) VBAProject.Module1, line 15
Warning: Member 'Cells' implicitly references ActiveSheet - (Book2) VBAProject.Module1, line 121
Warning: Member 'Cells' implicitly references ActiveSheet - (Book2) VBAProject.Module1, line 121
Warning: Member 'Cells' implicitly references ActiveSheet - (Book2) VBAProject.Module1, line 121
Suggestion: Consider renaming variable 'i' - (Book2) VBAProject.Module1, line 9
Suggestion: Consider renaming variable 'i' - (Book2) VBAProject.Module1, line 35
Suggestion: Consider renaming variable 'i' - (Book2) VBAProject.Module1, line 117
Suggestion: Consider renaming variable 'j' - (Book2) VBAProject.Module1, line 118
Warning: Variable 'i' is never assigned - (Book2) VBAProject.Module1, line 9
Warning: variable 'i' is not used - (Book2) VBAProject.Module1, line 9


Notice DealCards is declaring variable i which is neither assigned nor referred to, and can be safely removed.

DealCards would be more robust like this:

Public Sub DealCards()

    Dim target As Worksheet
    Set target = Sheet2 'Just dealing to sheet2
    target.Range("A:Z").Clear

    Dim numberOfPlayers As Long
    numberOfPlayers = GetPlayers
    If numberOfPlayers = 0 Then Exit Sub

    Dim myPlayers As Variant

    ReDim myPlayers(1 To numberOfPlayers, 1 To 6)    
    myPlayers = DealDeck(numberOfPlayers)

    With target
        .Range(.Cells(1, 1), .Cells(6, numberOfPlayers)) = Application.WorksheetFunction.Transpose(myPlayers)
    End With

    Colorize numberOfPlayers

End Sub

Now, Colorize also implicitly works off the ActiveSheet, so it would need to be given a Worksheet parameter:

Private Sub Colorize(ByVal numberofcolumns As Long, ByVal target As Worksheet)

And properly qualify its Cells calls with it.

StackExchange Code Review Q#132851, answer score: 4