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.
Private Function GetResultsRange(ByVal ws As Worksheet) As Range
Set GetResultsRange = ws.Range("K2:M50")
End Function
Now, note that we're referencing the column positions within the results range, instead of the position relative to the worksheet. This is easier for a person to comprehend while only looking at the code.
Public Sub Draw()
Dim ws As Worksheet
Set ws = ThisWorkbook.ActiveSheet
Dim resultsRange As Range
Set resultsRange = GetResultsRange(ws)
resultsRange.ClearContents
Dim numOfPlayers As Integer
numberOfPlayers = GetNumberOfPlayers()
Dim numberOfCivs As Integer
numberOfCivs = GetNumberOfCivilizations()
Dim i As Long
For i = 1 To numberOfPlayers
resultsRange.Cells(3 + (numberOfCivs + 2) * (i - 1), 1).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 resultsRange.Rows.Count
If resultsRange.Cells(z, 2) = RandCivilization Then
Exit For
End If
If z = resultsRange.Rows.Count Then
endOfRange = True
End If
Next z
Wend
resultsRange.Cells((numberOfCivs + 2) * (i - 1) + (j + 3), 2).Value = RandCivilization
Next j
Next i
End Sub
At this point, you should be getting the idea. Keep extracting well named functions until there's no duplicated or obscure logic left in your main routine. This snippet would be my next target.
(numberOfCivs + 2) * (i - 1)
As well as a couple of subs that are responsible for writing the data into the results range.
The code as it was in my IDE when I stopped reviewing:
Option Explicit
Public Sub Draw()
Dim ws As Worksheet
Set ws = ThisWorkbook.ActiveSheet
Dim resultsRange As Range
Set resultsRange = GetResultsRange(ws)
resultsRange.ClearContents
Dim numOfPlayers As Integer
numberOfPlayers = GetNumberOfPlayers()
Dim numberOfCivs As Integer
numberOfCivs = GetNumberOfCivilizations()
Dim i As Long
For i = 1 To numberOfPlayers
resultsRange.Cells(3 + (numberOfCivs + 2) * (i - 1), 1).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 resultsRange.Rows.Count
If resultsRange.Cells(z, 2) = RandCivilization Then
Exit For
End If
If z = resultsRange.Rows.Count Then
endOfRange = True
End If
Next z
Wend
resultsRange.Cells((numberOfCivs + 2) * (i - 1) + (j + 3), 2).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
Private Function GetNumberOfPlayers(ByVal ws As Worksheet) As Integer
GetNumberOfPlayers = ws.Cells(3, 3).Value
End Function
Private Function GetResultsRange(ByVal ws As Worksheet) As Range
Set GetResultsRange = ws.Range("K2:M50")
End Function
