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

Abusing Excel VBA... to maintain data stored in a database

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

Problem

I need to build a little UI to allow easy maintenance of some data stored in a MySQL database, and the end user wants to do that in Excel (I know, I know).

The code I came up with comes pretty close to what I'd call Model-View-Presenter (MVP).

I can get my form shown like this:

set x = new CustomerGroupsPresenter
set x.Command = new SqlCommand
x.Show


Obviously this code uses the SqlCommand ADODB wrapper that I've written earlier (for VB6), reviewable here.

Here's the form's code-behind:

Public Event CloseCommand(ByVal sender As CustomerGroupsView)
Public Event EditCustomerGroupCommand(ByVal id As Long, ByVal description As String)
Public Event AddCustomerGroupCommand()
Public Event DeleteCustomerGroupCommand(ByVal id As Long)
Option Explicit

Private Sub AddCustomerGroupButton_Click()
    RaiseEvent AddCustomerGroupCommand
End Sub

Private Sub CloseButton_Click()
    RaiseEvent CloseCommand(Me)
End Sub

Private Sub CustomerGroupsList_Change()
    DeleteCustomerGroupButton.Enabled = Not (CustomerGroupsList.ListIndex = -1)
End Sub

Private Sub CustomerGroupsList_DblClick(ByVal Cancel As MSForms.ReturnBoolean)

    Dim selectedRecord() As String
    selectedRecord = Split(CustomerGroupsList.List(CustomerGroupsList.ListIndex), StringFormat("\t"))

    Dim id As Long
    id = CLng(selectedRecord(0))

    Dim description As String
    description = selectedRecord(1)

    RaiseEvent EditCustomerGroupCommand(id, description)

End Sub

Private Sub DeleteCustomerGroupButton_Click()

    Dim selectedRecord() As String
    selectedRecord = Split(CustomerGroupsList.List(CustomerGroupsList.ListIndex), StringFormat("\t"))

    Dim id As Long
    id = CLng(selectedRecord(0))

    RaiseEvent DeleteCustomerGroupCommand(id)

End Sub


..and the Presenter class:

```
Private cmd As SqlCommand
Private WithEvents vwCustomerGroups As CustomerGroupsView
Option Explicit

Public Property Get Command() As SqlCommand
Set Command = cmd
End Property

Public Property Se

Solution

TL;DR

I wasn't going to review this, but I couldn't stop reading it again and again. So, here I am reviewing it. I'm going to go down line by line, but first let me address your question. Would creating a data service class be overkill? Right now, I think it would be. Overall this is a pretty clean implementation. Yes, the presenter is technically doing two things, but adding a data service just adds to the complexity. Add it when it makes sense to add it. YAGNI.

Another thing I would like to mention is that you're using a number of custom classes. Which is good. Developers should have a tool box. BUT.. the average VBA developer is not an actual developer. So, take into consideration who will be maintaining this after you've moved on. Will it be a business person who thinks they know how to program, or will it be a legitimate developer who has somehow been suckered into playing Mr. Maintainer for the project. (Let's face it, someone must have blackmailed him or her into it.) I think it makes a big difference on how much you rely on your toolbox that lets you program in a more C# type style.

Ok, on to the review...

Form's Code Behind

  • First, I'm going to nit-pick about something that really doesn't matter, but drives me crazy. Option Explicit should be the very first thing in any module. I'm happy to see you use it, but move it to where it belongs.



-
I think it is absolutely awesome that you're using custom object events. It's a highly under-utilized feature of the language. I really think you should go ahead and add a cancel parameter to all of these though. As a developer, I would want a way to bail out of Editing, Adding, and (especially) Deleting. Note that by using ByRef for the cancel parameter, we can "send" the cancel value back to the calling code so it can act appropriately if the event was canceled internally.

Public Event CloseCommand(ByVal sender As CustomerGroupsView, Optional ByRef Cancel As Boolean = False)
Public Event EditCustomerGroupCommand(ByVal id As Long, ByVal description As String, Optional ByRef Cancel As Boolean = False)
Public Event AddCustomerGroupCommand(Optional ByRef Cancel As Boolean = False)
Public Event DeleteCustomerGroupCommand(ByVal id As Long, Optional ByRef Cancel As Boolean = False)


Presenter

  • Same deal. Move Option Explicit to the first line.



  • Ditch the hungarian notation. You don't use it anywhere else, don't use it here. vwCustomerGroups is fine as just customerGroups. I do get why it was so tempting to do it though.



-
"\t" could be a constant TAB, but not a big deal.

-
vwCutstomerGroups_AddCustomerGroupCommand()

-
The inputbox code scrolls off the screen. This would be a legitimate use of line continuations.

Dim description As String
description = InputBox("Please enter a description for the new CustomerGroup:", _
    "Edit" , "(new customer group)")


  • It's hard to know when to use a one line If statement. I think this one is ok, but there are others that aren't.



  • I like seeing your use of proper parameterized queries.



-
vwCustomerGroups_DeleteCustomerGroupCommand

-
The message box code scrolls off my screen again. This time I'll offer an alternative way to utilize line continuations. I personally think this is a cleaner way to do it.

MsgBox _
    Prompt:=StringFormat("This CustomerGroup has {0} customer(s) associated to it and cannot be deleted.", childRecords), _
    Buttons:=vbExclamation, _
    Title:="FK Constraint violation!"


-
I understand from chat that your RDBMS doesn't support foreign keys. You can fake foreign keys with a trigger if you choose to do so. Triggers are typically evil, but I think this would be an acceptable use of them. Your database should be enforcing the data integrity, not your application.

-
This is a heck of a line of code here. Not only does it scroll off of the screen, it's an if statement. This one is not ok to leave as a one liner.

If MsgBox(StringFormat("Delete CustomerGroup #{0}?\n(this cannot be undone!)", id), vbExclamation + vbYesNo, "Please confirm") = vbNo Then Exit Sub

Code Snippets

Public Event CloseCommand(ByVal sender As CustomerGroupsView, Optional ByRef Cancel As Boolean = False)
Public Event EditCustomerGroupCommand(ByVal id As Long, ByVal description As String, Optional ByRef Cancel As Boolean = False)
Public Event AddCustomerGroupCommand(Optional ByRef Cancel As Boolean = False)
Public Event DeleteCustomerGroupCommand(ByVal id As Long, Optional ByRef Cancel As Boolean = False)
Dim description As String
description = InputBox("Please enter a description for the new CustomerGroup:", _
    "Edit" , "(new customer group)")
MsgBox _
    Prompt:=StringFormat("This CustomerGroup has {0} customer(s) associated to it and cannot be deleted.", childRecords), _
    Buttons:=vbExclamation, _
    Title:="FK Constraint violation!"
If MsgBox(StringFormat("Delete CustomerGroup #{0}?\n(this cannot be undone!)", id), vbExclamation + vbYesNo, "Please confirm") = vbNo Then Exit Sub

Context

StackExchange Code Review Q#57734, answer score: 4

Revisions (0)

No revisions yet.