# 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.

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

• Admit it, you wanted to be the first to paste Rubberduck code inspection results in a CR answer! ;) Mar 8 '15 at 23:34
• That's about as done as my review will get @SimonKarfunkel. It could be a bit buggy. I didn't mock out the data and run it. Mar 9 '15 at 9:44

Columns $O:$P are application data, they don't quite belong on your front-end worksheet.

I'd copy these two columns over to another worksheet, insert a header row, add headers CivilizationName and LeaderName, and then convert that range to a table. What gives? Tables are one of the most useful & powerful features of Excel, might as well use them!

On the VBA side of things, it greatly simplifies accessing the data - you can give a programmatic name to a worksheet, so I'd name the worksheet with the table Civilizations, and name the table itself tblCivilizations (the "tbl" prefix is useful to differenciate different types of named ranges in Excel formulas).

This means VBA can now access the table like this:

Dim civilizationsTable As ListObject
Set civilizationsTable = Civilizations.ListObjects(1)


And then the rows can be iterated with civilizationsTable.ListRows, which means you can get the value in the 2nd column at row i like this:

civilizationsTable.ListRows(i).Range(ColumnIndex:=2)


...which means a line like this:

Rand_CIV = Cells(RandNum, 16).Value & " (" & Cells(RandNum, 15).Value & ")"


Could be written like this:

civName = civilizationsTable.ListRows(RandNum).Range(ColumnIndex:=1)
civCaption = civLeader & " (" & civName & ")"


More code, you'll say? Wait, I'm not done yet.

Public Enum CivilizationTableColumns
CivilizationName = 1
End Enum


Turns it into this:

civName = civilizationsTable.ListRows(RandNum).Range(ColumnIndex:=CivilizationName)
civCaption = civLeader & " (" & civName & ")"


And then we can afford a ListRow object:

Set row = civilizationsTable.ListRows(RandNum)
civName = row.Range(ColumnIndex:=CivilizatioName)
civCaption = civLeader & " (" & civName & ")"


Magic numbers? What magic numbers? ...and then that little chunk can be extracted into its own little function:

Private Function GetCivilizationCaption(ByVal index As Long)
Set row = civilizationsTable.ListRows(index)
civName = row.Range(ColumnIndex:=CivilizatioName)

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

AssignCivilizationForPlayer player, civilizationCaption