19
\$\begingroup\$

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.

Cropped screenshot of spreadsheet showing how code works

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.

\$\endgroup\$
0
12
\$\begingroup\$

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
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Admit it, you wanted to be the first to paste Rubberduck code inspection results in a CR answer! ;) \$\endgroup\$ Mar 8 '15 at 23:34
  • \$\begingroup\$ 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. \$\endgroup\$
    – RubberDuck
    Mar 9 '15 at 9:44
11
\$\begingroup\$

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)
civLeader = civilizationsTable.ListRows(RandNum).Range(ColumnIndex:=2)
civCaption = civLeader & " (" & civName & ")"

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

Public Enum CivilizationTableColumns
    CivilizationName = 1
    CivilizationLeader '= 2
End Enum

Turns it into this:

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

And then we can afford a ListRow object:

Set row = civilizationsTable.ListRows(RandNum)
civName = row.Range(ColumnIndex:=CivilizatioName)
civLeader = row.Range(ColumnIndex:=CivilizationLeader)
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)
    civLeader = row.Range(ColumnIndex:=CivilizationLeader)
    GetCivilizationCaption = civLeader & " (" & civName & ")"
End Function

The takeaway here, is that it's hard to see what's going on at a glance, because of the lack of abstraction. Our puny little human brains love abstractions. Proof - what would you rather be maintaining, 6 months down the line?

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

Or..

AssignCivilizationForPlayer player, civilizationCaption
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.