Randomizing Civilization 5 team choice

This macro is made to randomly draw specific number of civilization choices for a specific number of Civ V players. It is based on a list of 43 possibilities and the for loop in the while loop checks for already picked civilizations. Constant cell references point to values obtained with scroll-bars in the worksheet. The variable based cell references are made to structure the results in a two-column design.

Are there any smart ways to make my code better from both an aesthetic as well as functional view?

Sub CIV_draw()
Range("K2:M50").ClearContents
Dim x As Integer
x = Cells(3, 3).Value
For i = 1 To x
    Cells(3 + (Cells(3, 7).Value + 2) * (i - 1), 11).Value = "Player " & i
    For j = 1 To Cells(3, 7).Value
        Dim Rand_CIV As String
        Dim RandNum As Integer
        Dim checkVal As Boolean
        checkVal = False
        While checkVal = False
            RandNum = CInt(Int(43 * Rnd())) + 1
            Rand_CIV = Cells(RandNum, 16).Value & " (" & Cells(RandNum, 15).Value & ")"
            For Z = 1 To 50
                If Cells(Z, 12) = Rand_CIV Then
                    Exit For
                End If
                If Z = 50 Then
                    checkVal = True
                End If
            Next Z
        Wend

        Cells((Cells(3, 7).Value + 2) * (i - 1) + (j + 3), 12).Value = Rand_CIV
    Next j
Next i
End Sub

I know for example that my duplicate check isn't optimal, and I would like to get suggestions for alternative solutions.

Well, Rubberduck only indicates one issue.


Rubberduck Code Inspections - 3/8/2015 12:56:11 PM
1 issue found.
Suggestion: Member 'CIV_draw' is implicitly Public - VBAProject.Module1, line 3

Implicit Public Member documentation

So, you're off to a good start. You can correct this by explicitly declaring the Subroutine's scope.

Public Sub CIV_draw()

Personally, I would also ditch the prefix and just call this routine Draw(). The underscore holds a special place in VBA. It typically denotes either an Event procedure or an Interface implementation, so I would avoid using underscores in procedure names elsewhere.

The next thing I notice is that you're calling the Range() and Cells() methods without specifying a worksheet. This means you're implicitly calling these methods on the ActiveSheet. This should be avoided because it leaves a small chance that the user will change the worksheet while the code is executing. You should instead create a worksheet variable that gets set at the very beginning, then work with that instead.

Dim ws As Worksheet
Set ws = ThisWorkbook.ActiveSheet

ws.Range("K2:M50").ClearContents
' ...

There are also a lot of magic numbers in your code, but I'll get back to that. First, let's talk about extracting some well named methods. The random number logic is a good one to extract out. When reading this method, we don't care really how the random number is generated, only that we are getting one.

RandNum = CInt(Int(43 * Rnd())) + 1

Private Function GetRandomNum() As Integer
    GetRandomNum = CInt(Int(43 * Rnd())) + 1
End Function

Next is the business of getting a random civilization string.

Private Function GetRandomCivilization(ByVal RandNum As Integer, ByVal ws As Worksheet) As String
    GetRandomCivilization = ws.Cells(RandNum, 16).Value & " (" & ws.Cells(RandNum, 15).Value & ")"
End Function

This is already making your code more readable at a high level. If anyone wants the details, they can dig into these private functions. We've also entirely eliminated a variable by doing this.

Dim RandCivilization As String
Dim checkVal As Boolean

checkVal = False
While checkVal = False
    RandCivilization = GetRandomCivilization(GetRandomNum(), ws)
    For Z = 1 To 50

Next let's clean up your While loop a bit.

Dim checkVal As Boolean

checkVal = False
While checkVal = False

Next, let's flip the logic to use a Not condition.

Dim checkVal As Boolean
checkVal = False

While Not checkVal

And finally, give it a better name.

Dim endOfRange As Boolean
endOfRange = False

While Not endOfRange

The next order of business is to extract this duplicated logic into its own method.

(ws.Cells(3, 7).Value + 2)

Private Function GetNumberOfCivilizations(ByVal ws As Worksheet)
    GetNumberOfCivilizations = ws.Cells(3, 7).Value
End Function

Which brings the current state of the code to this.

Public Sub Draw()
    Dim ws As Worksheet
    Set ws = ThisWorkbook.ActiveSheet

    ws.Range("K2:M50").ClearContents

    Dim x As Integer
    x = ws.Cells(3, 3).Value

    Dim numberOfCivs As Integer
    numberOfCivs = GetNumberOfCivilizations()

    Dim i As Long
    For i = 1 To x
        ws.Cells(3 + (numberOfCivs + 2) * (i - 1), 11).Value = "Player " & i

        Dim j As Long
        For j = 1 To numberOfCivs
            Dim RandCivilization As String
            Dim endOfRange As Boolean

            endOfRange = False
            While Not endOfRange
                RandCivilization = GetRandomCivilization(GetRandomNum(), ws)

                Dim z As Long
                For z = 1 To 50
                    If ws.Cells(z, 12) = RandCivilization Then
                        Exit For
                    End If
                    If z = 50 Then
                        endOfRange = True
                    End If
                Next z
            Wend

            ws.Cells((numberOfCivs + 2) * (i - 1) + (j + 3), 12).Value = RandCivilization
        Next j
    Next i
End Sub

Private Function GetNumberOfCivilizations(ByVal ws As Worksheet)
    GetNumberOfCivilizations = ws.Cells(3, 7).Value
End Function

Private Function GetRandomNum() As Integer
    GetRandomNum = CInt(Int(43 * Rnd())) + 1
End Function

Private Function GetRandomCivilization(ByVal RandNum As Integer, ByVal ws As Worksheet) As String
    GetRandomCivilization = ws.Cells(RandNum, 16).Value & " (" & ws.Cells(RandNum, 15).Value & ")"
End Function

That's an improvement, but we're still dealing with a lot of numbers that don't mean much to us until we reference back to the actual worksheet. Let's define constant values for them and some functions that are responsible for getting data from the worksheet.

Private Function GetNumberOfPlayers(ByVal ws As Worksheet) As Integer
    GetNumberOfPlayers = ws.Cells(3, 3).Value
End Function

And yet another function to get the range we're writing the results to.

```
Priva

StackExchange Code Review Q#83573, answer score: 12