4
\$\begingroup\$

I am trying to implement the game 2048 in Excel VBA.

Figure1

The each TRUE / FALSE Boolean value in row 2, 4, 6 and 8 are used for determining the data of each cell in row 1, 3, 5 and 7 is 0 or not.

The experimental implementation

Sub MergeUp()
    Dim loop_num
    Dim loop_num2
    For loop_num2 = 1 To 3
        For loop_num = 1 To 4
            If Cells(6, loop_num) = True Then
                Cells(5, loop_num) = Cells(7, loop_num)
                Cells(7, loop_num) = 0
            End If
            If Cells(4, loop_num) = True Then
                Cells(3, loop_num) = Cells(5, loop_num)
                Cells(5, loop_num) = 0
            End If
            If Cells(2, loop_num) = True Then
                Cells(1, loop_num) = Cells(3, loop_num)
                Cells(3, loop_num) = 0
            End If
        Next
    Next
    For loop_num = 1 To 4
        If Cells(1, loop_num) = Cells(3, loop_num) Then
            Cells(1, loop_num) = Cells(1, loop_num) + Cells(3, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(3, loop_num)
            Cells(3, loop_num) = 0
        End If
        If Cells(3, loop_num) = Cells(5, loop_num) Then
            Cells(3, loop_num) = Cells(3, loop_num) + Cells(5, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(5, loop_num)
            Cells(5, loop_num) = 0
        End If
        If Cells(5, loop_num) = Cells(7, loop_num) Then
            Cells(5, loop_num) = Cells(5, loop_num) + Cells(7, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(7, loop_num)
            Cells(7, loop_num) = 0
        End If
    Next
    For loop_num2 = 1 To 3
        For loop_num = 1 To 4
            If Cells(6, loop_num) = True Then
                Cells(5, loop_num) = Cells(7, loop_num)
                Cells(7, loop_num) = 0
            End If
            If Cells(4, loop_num) = True Then
                Cells(3, loop_num) = Cells(5, loop_num)
                Cells(5, loop_num) = 0
            End If
            If Cells(2, loop_num) = True Then
                Cells(1, loop_num) = Cells(3, loop_num)
                Cells(3, loop_num) = 0
            End If
        Next
    Next
    Call rand_num
End Sub
Sub MergeDown()
    Dim loop_num
    Dim loop_num2
    For loop_num2 = 1 To 3
        For loop_num = 1 To 4
            If Cells(4, loop_num) = True Then
                Cells(3, loop_num) = Cells(1, loop_num)
                Cells(1, loop_num) = 0
            End If
            If Cells(6, loop_num) = True Then
                Cells(5, loop_num) = Cells(3, loop_num)
                Cells(3, loop_num) = 0
            End If
            If Cells(8, loop_num) = True Then
                Cells(7, loop_num) = Cells(5, loop_num)
                Cells(5, loop_num) = 0
            End If
        Next
    Next
    For loop_num = 1 To 4
        If Cells(7, loop_num) = Cells(5, loop_num) Then
            Cells(7, loop_num) = Cells(7, loop_num) + Cells(5, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(5, loop_num)
            Cells(5, loop_num) = 0
        End If
        If Cells(5, loop_num) = Cells(3, loop_num) Then
            Cells(5, loop_num) = Cells(5, loop_num) + Cells(3, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(3, loop_num)
            Cells(3, loop_num) = 0
        End If
        If Cells(3, loop_num) = Cells(1, loop_num) Then
            Cells(3, loop_num) = Cells(3, loop_num) + Cells(1, loop_num)
            Cells(9, "C") = Cells(9, "C") + Cells(1, loop_num)
            Cells(1, loop_num) = 0
        End If
    Next
    For loop_num2 = 1 To 3
        For loop_num = 1 To 4
            If Cells(4, loop_num) = True Then
                Cells(3, loop_num) = Cells(1, loop_num)
                Cells(1, loop_num) = 0
            End If
            If Cells(6, loop_num) = True Then
                Cells(5, loop_num) = Cells(3, loop_num)
                Cells(3, loop_num) = 0
            End If
            If Cells(8, loop_num) = True Then
                Cells(7, loop_num) = Cells(5, loop_num)
                Cells(5, loop_num) = 0
            End If
        Next
    Next
    Call rand_num
End Sub
Sub MergeLeft()
    For loop_num2 = 1 To 3
        For loop_num = 1 To 7 Step 2
            If Cells(loop_num + 1, "C") = True Then
                Cells(loop_num, "C") = Cells(loop_num, "D")
                Cells(loop_num, "D") = 0
            End If
            If Cells(loop_num + 1, "B") = True Then
                Cells(loop_num, "B") = Cells(loop_num, "C")
                Cells(loop_num, "C") = 0
            End If
            If Cells(loop_num + 1, "A") = True Then
                Cells(loop_num, "A") = Cells(loop_num, "B")
                Cells(loop_num, "B") = 0
            End If
        Next
    Next
    For loop_num = 1 To 7 Step 2
        If Cells(loop_num, "A") = Cells(loop_num, "B") Then
            Cells(loop_num, "A") = Cells(loop_num, "A") + Cells(loop_num, "B")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "B")
            Cells(loop_num, "B") = 0
        End If
        If Cells(loop_num, "B") = Cells(loop_num, "C") Then
            Cells(loop_num, "B") = Cells(loop_num, "B") + Cells(loop_num, "C")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "C")
            Cells(loop_num, "C") = 0
        End If
        If Cells(loop_num, "C") = Cells(loop_num, "D") Then
            Cells(loop_num, "C") = Cells(loop_num, "C") + Cells(loop_num, "D")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "D")
            Cells(loop_num, "D") = 0
        End If
    Next
    For loop_num2 = 1 To 3
        For loop_num = 1 To 7 Step 2
            If Cells(loop_num + 1, "C") = True Then
                Cells(loop_num, "C") = Cells(loop_num, "D")
                Cells(loop_num, "D") = 0
            End If
            If Cells(loop_num + 1, "B") = True Then
                Cells(loop_num, "B") = Cells(loop_num, "C")
                Cells(loop_num, "C") = 0
            End If
            If Cells(loop_num + 1, "A") = True Then
                Cells(loop_num, "A") = Cells(loop_num, "B")
                Cells(loop_num, "B") = 0
            End If
        Next
    Next
    Call rand_num
End Sub
Sub MergeRight()
    For loop_num2 = 1 To 3
        For loop_num = 1 To 7 Step 2
            If Cells(loop_num + 1, "B") = True Then
                Cells(loop_num, "B") = Cells(loop_num, "A")
                Cells(loop_num, "A") = 0
            End If
            If Cells(loop_num + 1, "C") = True Then
                Cells(loop_num, "C") = Cells(loop_num, "B")
                Cells(loop_num, "B") = 0
            End If
             If Cells(loop_num + 1, "D") = True Then
                Cells(loop_num, "D") = Cells(loop_num, "C")
                Cells(loop_num, "C") = 0
            End If
        Next
    Next
    For loop_num = 1 To 7 Step 2
        If Cells(loop_num, "C") = Cells(loop_num, "D") Then
            Cells(loop_num, "D") = Cells(loop_num, "D") + Cells(loop_num, "C")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "C")
            Cells(loop_num, "C") = 0
        End If
        If Cells(loop_num, "B") = Cells(loop_num, "C") Then
            Cells(loop_num, "C") = Cells(loop_num, "C") + Cells(loop_num, "B")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "B")
            Cells(loop_num, "B") = 0
        End If
        If Cells(loop_num, "A") = Cells(loop_num, "B") Then
            Cells(loop_num, "B") = Cells(loop_num, "B") + Cells(loop_num, "A")
            Cells(9, "C") = Cells(9, "C") + Cells(loop_num, "A")
            Cells(loop_num, "A") = 0
        End If
    Next
    For loop_num2 = 1 To 3
        For loop_num = 1 To 7 Step 2
            If Cells(loop_num + 1, "B") = True Then
                Cells(loop_num, "B") = Cells(loop_num, "A")
                Cells(loop_num, "A") = 0
            End If
            If Cells(loop_num + 1, "C") = True Then
                Cells(loop_num, "C") = Cells(loop_num, "B")
                Cells(loop_num, "B") = 0
            End If
             If Cells(loop_num + 1, "D") = True Then
                Cells(loop_num, "D") = Cells(loop_num, "C")
                Cells(loop_num, "C") = 0
            End If
        Next
    Next
    Call rand_num
End Sub
Public Sub rand_num()
    Dim cell_row(4)
    cell_row(1) = 1
    cell_row(2) = 3
    cell_row(3) = 5
    cell_row(4) = 7
    Dim rand_number(2)
    rand_number(1) = cell_row(Int((4 - 1 + 1) * Rnd + 1))
    rand_number(2) = Int((4 - 1 + 1) * Rnd + 1)
    While (Cells(rand_number(1) + 1, rand_number(2)) = False)
    rand_number(1) = cell_row(Int((4 - 1 + 1) * Rnd + 1))
    rand_number(2) = Int((4 - 1 + 1) * Rnd + 1)
    Wend
    Cells(rand_number(1), rand_number(2)) = Int((2 - 1 + 1) * Rnd + 1)
    Randomize [Timer]
End Sub
Sub Clear()
    For loop_num = 1 To 4
        Cells(1, loop_num) = 0
        Cells(3, loop_num) = 0
        Cells(5, loop_num) = 0
        Cells(7, loop_num) = 0
    Next
    Cells(9, "C") = 0
    Call rand_num
    Call rand_num
End Sub

All suggestions are welcome.

\$\endgroup\$
1
  • \$\begingroup\$ micro review:); =IF(A1=0,TRUE,FALSE) is equivalent to =A1=0 (or =(A1=0) if you're an anxious sort) \$\endgroup\$ – Greedo Mar 6 at 15:51
5
\$\begingroup\$

Some quick feedback because I think you can make some small changes to really improve the readability of your code. If you decide to make some of them you can then post a new question with updated code to get some more targeted reviews...

Variables

Dim loop_num
Dim loop_num2
Dim rand_number(2) 'etc.

Although not explicitly required, variables in VBA can have types, just like in strongly typed languages such as c++. Including types in code has 2 benefits:

  • Improves performance, as typed variables work faster and take up less memory
  • Perhaps more importantly, helps to document the code; if we know loop_num is an integer or rand_num is floating point, then we can assume some stuff about what they might be used for. This makes reading, maintaining and improving the code much easier.

So prefer:

Dim loop_num As Long
Dim loop_num2 As Long
Dim rand_number(2) As Single

While we're at it, those variable names aren't very descriptive are they? Sure loop_num tells me it's probably the incremented variable in a for-loop, but I can already see that just by looking! It's better to use descriptive names that make the code self-documenting and easy to follow. I don't really know what all those variables are for, as I'm focussing on the general problems for now, but maybe something like:

Dim worksheetRow As Long
Dim iterationPassNumber As Long

...would be better.

Also in a couple of places you don't declare variables; aside from meaning you can't declare a type, failing to enforce variable declarations can allow typos to slip through, loop_counter vs loop_cuonter. Add Option Explicit at the top of your module(s) to enforce variable declaration and help you pick up on typos.


Magic Numbers

Your code contains a lot of magic numbers - literal values that don't mean anything in isolation but which have some special meaning in your code.

Dim cell_row(4)
cell_row(1) = 1    'ok I guess, thanks to a fairly descriptive variable name
cell_row(2) = 3
cell_row(3) = 5
cell_row(4) = 7
Dim rand_number(2)   'um sure, 2 of them why not
rand_number(1) = cell_row(Int((4 - 1 + 1) * Rnd + 1))   'I'm at a loss... What do these numbers mean?!
rand_number(2) = Int((4 - 1 + 1) * Rnd + 1)
While (Cells(rand_number(1) + 1, rand_number(2)) = False)
rand_number(1) = cell_row(Int((4 - 1 + 1) * Rnd + 1))
rand_number(2) = Int((4 - 1 + 1) * Rnd + 1)
Wend
Cells(rand_number(1), rand_number(2)) = Int((2 - 1 + 1) * Rnd + 1)

Imagine reading this code for the first time (as I am right now) - would you have any idea what that function was doing. I see random numbers being put in cells, some loop that looks like it might never stop, I have no idea what the logic of this code is supposed to be. I mean

Int((4 - 1 + 1) ' * [...]

Why??

Adding comments to explain why your code is doing what it's doing, or better yet, renaming those numbers as constants:

Const randomNumberScalingFactor As Long = 4 'or 4 + SomeMagicNumber - AnotherMagicNumber

... then (for example)

randomColumnIndexInSheet = Int(ColumnCount * Rnd + ColumnOffset))

DRY

Don't Repeat Yourself; MergeUp/Down andMergeLeft/Right contain a lot of repetition, but with slightly different combinations of A,B,C,D. It would be better to take these as arguments to a single Sub, so you can reuse the code to do multiple things.

For example:

For loop_num2 = 1 To 3
    For loop_num = 1 To 7 Step 2
        If Cells(loop_num + 1, "B") = True Then
            Cells(loop_num, "B") = Cells(loop_num, "A")
            Cells(loop_num, "A") = 0
        End If
        If Cells(loop_num + 1, "C") = True Then
            Cells(loop_num, "C") = Cells(loop_num, "B")
            Cells(loop_num, "B") = 0
        End If
         If Cells(loop_num + 1, "D") = True Then
            Cells(loop_num, "D") = Cells(loop_num, "C")
            Cells(loop_num, "C") = 0
        End If
    Next

For loop_num2 = 1 To 3
    For loop_num = 1 To 7 Step 2
        If Cells(loop_num + 1, "C") = True Then
            Cells(loop_num, "C") = Cells(loop_num, "D")
            Cells(loop_num, "D") = 0
        End If
        If Cells(loop_num + 1, "B") = True Then
            Cells(loop_num, "B") = Cells(loop_num, "C")
            Cells(loop_num, "C") = 0
        End If
        If Cells(loop_num + 1, "A") = True Then
            Cells(loop_num, "A") = Cells(loop_num, "B")
            Cells(loop_num, "B") = 0
        End If
    Next
Next

could become something like:

Sub MoveSidewaysWithMapping(ByVal first As String, ByVal second As String, ByVal third As String, ByVal fourth As String)
    '[...]
    For loop_num2 = 1 To 3
        For loop_num = 1 To 7 Step 2
            If Cells(loop_num + 1, third) = True Then
                Cells(loop_num, third) = Cells(loop_num, fourth)
                Cells(loop_num, fourth) = 0
            End If
            If Cells(loop_num + 1, second) = True Then
                Cells(loop_num, second) = Cells(loop_num, third)
                Cells(loop_num, third) = 0
            End If
            If Cells(loop_num + 1, first) = True Then
                Cells(loop_num, first) = Cells(loop_num, second)
                Cells(loop_num, second) = 0
            End If
        Next
    Next
    'etc...

called like

'move left:
 MoveSidewaysWithMapping "A","B","C","D"

'move right
 MoveSidewaysWithMapping "D","C","B","A"

you get the idea (p.s. the Call keyword as in Call rand_num is obsolete, you don't need it anymore, and it's good to remove IMO because it's excess clutter for your brain to process)

Re-using code is valuable as it means if you change the logic, you only change it in one place making it less bug prone. Also less code to process probably makes the code easier to interpret for reviewers and maintainers (you in 6 months), as long as shortening doesn't reduce legibility (which in this case I don't think it would)


Anyway, that's an initial first pass, if you want more feedback about your technique and algorithm perhaps, then I'd recommend tidying your code up as much as possible with some of the above techniques, and posting a follow up question.

Hope that helps, let me know if you need clarification (PS, I'm not sure how much of that will be new to you as I see you've asked a lot of questions on CR already, and this is pretty general/basic advice, but I understand if you're just dabbling with a newish language, you can spend most of your time trying to get it work and forget some of the important stylistic fundamentals!)

\$\endgroup\$
5
  • \$\begingroup\$ Capitalize constants and all those implicit Activesheet references are a bug fest. \$\endgroup\$ – QHarr Mar 9 at 3:45
  • \$\begingroup\$ @QHarr oh yeah, haven't mentioned a number of things here, I was waiting for a follow up post that fixes these even more low-hanging points I mention, to make life easier for reviewers. What do you mean about "Capitalize constants"? You think I should write them YELLCASE or are you saying that the "A","B","C","D" arguments are fraught with risk? I've never really been convinced either way on yellcase since I tend to prefer descriptive names and YELLCASEWITHOUTUNDERSCORES is hard to read, with underscores feels like a clash with IInterface_ImplementationMethods \$\endgroup\$ – Greedo Mar 9 at 9:03
  • \$\begingroup\$ e.g. randomColumnIndexInSheet vs RANDOMCOLUMNINDEXINSHEET \$\endgroup\$ – Greedo Mar 9 at 9:06
  • 1
    \$\begingroup\$ lol! I get what you mean and I think yelling with underscores is the common middle ground. \$\endgroup\$ – QHarr Mar 9 at 11:16
  • 2
    \$\begingroup\$ @QHarr yeah, but that's what I'm saying, YELL_WITH_UNDERSCORES looks great I agree, but in VBA _ has special meaning for names (indicates interface implementation) which it doesn't in other languages. E.g. the python convention for naming constants is as you describe, but they also use snake_case for function names which is generally frowned upon in VBA - because an _ in a function means that function can't be implemented (IIRC). So generally I avoid underscores, and therefore prefer PascalCase for public and camelCase for private consts \$\endgroup\$ – Greedo Mar 12 at 12:45
2
\$\begingroup\$

Helper Variables and Cells

Helper variables and cells should be used to better describe simplify our code. The cell formulas are complicating the process. They are forcing you to process Merge Up and Merge Down differently from Merge Left and Merge Right. Use a simple 4 x 4 matrix instead.

Using a 4 x 4 matrix would allow you to load the data into an array, process it in memory, and overwrite the original data. Not only is this more efficient but it separates the data model from the data view. You will be able to apply the same logic whether the game board starts in cell A1 or Z100.

Use Fully Qualified References

It is a best practice to qualify your cell references to a worksheet. This makes it easier to debug and reuse your code.

Here is how I would setup the borad to use a 4 x 4 matrix:

Private Const GameSheetName As String = "2048"
Private Const TopLeftCellAddress As String = "A1"
Private Const ColumnCount As Long = 4
Private Const RowCount As Long = 4

Function GameSheet() As Worksheet
    Set GameBoard = ThisWorkbook.Worksheets(GameSheetName)
End Function

Function GameBoard() As Range
    Set GameBoard = GameSheet.Range(TopLeftCellAddress).Resize(RowCount, ColumnCount)
End Function

Function Scrore() As Range
    Set GameBoard = GameBoard.Offset(RowCount, ColumnCount).Offset(2, 3)
End Function

Merge Numbers

I might be missing something but shouldn't the loop decrement 1 so that the next value can be moved and possibly merged?

Don't Repeat Yourself Principle (DRY)

As Greedo correctly states, you should try and avoid repeat code. I recommend passing an enumerated value into a main subroutine and having it process the data.

In this main procedure I would have two nested loops. The enum would be used to determine the start, end and step values of the loops.

Private Enum MergeDirection
    Left
    Right
    Up
    Down
End Enum

Sub MergeTiles(Direction As MergeDirection)
    Dim Data As Variant
    Data = GameBoard.Value
    Dim a As Long, b As Long
    
    Rem Loop Logic
    
    GameBoard.Value = Data
End Sub
\$\endgroup\$

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.