HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Excel display driver class for Tetris

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
excelfordriverdisplayclasstetris

Problem

This is the first of (hopefully) several posts as I reimplement an ancient (and unfortunately long lost) Excel VBA Tetris clone. The main workhorse class in the game engine is the display driver, which uses a rectangular area of a Worksheet to emulate a monitor. The display is sprite based, so the class holds a Dictionary loaded with IDrawables, which are basically bitmaps. The interface is as follows:

'IDrawable.cls
Option Explicit

Public Property Get Top() As Long
End Property

Public Property Let Top(inValue As Long)
End Property

Public Property Get Left() As Long
End Property

Public Property Let Left(inValue As Long)
End Property

Public Property Get Width() As Long
End Property

Public Property Get Height() As Long
End Property

Public Property Let Bitmap(colors() As Long)
End Property

Public Property Get Bitmap() As Long()
End Property

Public Function GetPixel(pixelRow As Long, pixelColumn As Long) As Long
End Function


Properties

AnchorCell - This is a one cell Range that determines where the display rectangle will be placed. It will raise an error if more than one cell is passed.

Top and Left are the row and column number of the AnchorCell in the parent Worksheet's coordinates. These are read-only and are derived from the AnchorCell.

Width and Height set the dimensions of the display.

DotPitch is the size of each "pixel", and the rows and columns in the display range are set to this width and height (in pixels). Currently I have it limited to CGA resolution (mainly for convenience when I test it), but the limits can be set by altering the appropriate constants.

BackColor sets the background color for the display. Any pixel in an IDrawable that matches the BackColor will be "transparent".

Methods

AddDrawable, RemoveDrawable and ClearDrawables are the main methods for interacting with the display. Any item added to the container will be rendered.

Refresh forces the display to repaint itself. If change

Solution

First let me say that this is pure awesome, and I only have superficial improvements to suggest. nevermind, this grew bigger than I originally thought.... again.

There is no reason for any of the parameters (well, except the array one) you're passing any of the IDrawable interface members to be passed ByRef - and they're all passed by reference, implicitly.

Public Property Let Top(inValue As Long)
End Property

Public Property Let Left(inValue As Long)
End Property

Public Function GetPixel(pixelRow As Long, pixelColumn As Long) As Long
End Function


Could be:

Public Property Let Top(ByVal inValue As Long)
End Property

Public Property Let Left(ByVal inValue As Long)
End Property

Public Function GetPixel(ByVal pixelRow As Long, ByVal pixelColumn As Long) As Long
End Function


While we're at it, might as well make the colors() array explicitly passed ByRef:

Public Property Let Bitmap(ByRef colors() As Long)
End Property


In the Sprite class (I know, it's test/demo code), there's not really a need to expose any public members - in fact the only reason you do need them is because you've declared foo As Sprite:

Dim foo As Sprite
Set foo = New Sprite


But this would work just as well:

Dim foo As IDrawable
Set foo = New Sprite


Actually, I think the IDrawable interface should not expose Property Let members to mutate Top and Left values (but leave them on the concrete implementations... or don't - and give it a PredeclaredId and a Create function instead). It could expose some Move procedure instead of mutators:

Public Sub Move(ByVal x As Long, ByVal y As Long)
End Sub


That would change this client code:

If right Then
    .Left = .Left + 1
Else
    .Left = .Left - 1
End If
If down Then
    .Top = .Top + 1
Else
    .Top = .Top - 1
End If


Into that one-liner:

.Move IIf(right, 1, -1), IIf(down, 1, -1)


The Refresh method feels a bit cluttered, because it's responsible for drawing every pixel of every drawable object. You could have some DrawableObject with a PredeclaredId, and a "static" method where you could draw one single IDrawable object:

Public Sub Draw(ByRef drawable As IDrawable, ByRef screen As Range)
    Dim r As Long
    Dim c As Long
    Dim rowTarget As Long
    Dim colTarget As Long
    For r = 1 To drawable.Height
        rowTarget = r + drawable.Top
        If rowTarget >= 1 And rowTarget = 1 And colTarget < this.Width Then
                    screen.Cells(rowTarget, colTarget).Interior.Color = drawable.GetPixel(r, c)
                End If
            Next
        End If
    Next
End Sub


Then this:

With this.DisplayArea
    .Interior.Color = this.BackColor
    Dim drawable As Variant
    For Each drawable In this.Drawables.Keys
        Dim r As Long
        Dim c As Long
        Dim rowTarget As Long
        Dim colTarget As Long
        For r = 1 To drawable.Height
            rowTarget = r + drawable.Top
            If rowTarget >= 1 And rowTarget = 1 And colTarget < this.Width Then
                        .Cells(rowTarget, colTarget).Interior.Color = drawable.GetPixel(r, c)
                    End If
                Next
            End If
        Next
    Next
End With


Becomes this:

With this.DisplayArea
    .Interior.Color = this.BackColor
    Dim drawable As Variant 'shame this can't be IDrawable.. right?
    For Each drawable In this.Drawables.Keys
        DrawableObject.Draw drawable, .Range 'might need to cast to IDrawable
    Next
End With

Code Snippets

Public Property Let Top(inValue As Long)
End Property

Public Property Let Left(inValue As Long)
End Property

Public Function GetPixel(pixelRow As Long, pixelColumn As Long) As Long
End Function
Public Property Let Top(ByVal inValue As Long)
End Property

Public Property Let Left(ByVal inValue As Long)
End Property

Public Function GetPixel(ByVal pixelRow As Long, ByVal pixelColumn As Long) As Long
End Function
Public Property Let Bitmap(ByRef colors() As Long)
End Property
Dim foo As Sprite
Set foo = New Sprite
Dim foo As IDrawable
Set foo = New Sprite

Context

StackExchange Code Review Q#141404, answer score: 5

Revisions (0)

No revisions yet.