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

Question

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 Set Command(ByRef value As SqlCommand)
 Set cmd = value
End Property
Public Sub Show()
 Set vwCustomerGroups = New CustomerGroupsView
 RefreshCustomerGroups
 vwCustomerGroups.Show vbModal
End Sub
Private Function View() As CustomerGroupsView
 Set View = vwCustomerGroups
End Function
Private Sub RefreshCustomerGroups()
 vwCustomerGroups.CustomerGroupsList.Clear
 Dim sql As String
 sql = "SELECT Id, Description FROM Planning.CustomerGroups ORDER BY Id;"
 Dim groups As SqlResult
 Set groups = cmd.QuickExecute(sql)
 groups.ValueSeparator = StringFormat("\t")
 Dim group As SqlResultRow
 For Each group In groups
 vwCustomerGroups.CustomerGroupsList.AddItem group.ToString
 Next
End Sub
Private Sub vwCustomerGroups_AddCustomerGroupCommand()
 Dim description As String
 description = InputBox("Please enter a description for the new CustomerGroup:", "Edit", "(new customer group)")
 If StrPtr(description) = 0 Or description = vbNullString Then Exit Sub
 Dim sql As String
 sql = "INSERT INTO Planning.CustomerGroups (Description, DateInserted) VALUES (?, ?);"
 Dim success As Boolean
 success = cmd.QuickExecuteNonQuery(sql, description, Now)
 If Not success Then MsgBox "Insert operation failed!", vbExclamation, "Warning"
 RefreshCustomerGroups
End Sub
Private Sub vwCustomerGroups_CloseCommand(ByVal sender As CustomerGroupsView)
 sender.Hide
End Sub
Private Sub vwCustomerGroups_DeleteCustomerGroupCommand(ByVal id As Long)
 Dim sql As String
 sql = "SELECT COUNT(*) FROM Planning.Customers WHERE CustomerGroupId = ?;"
 Dim childRecords As Long
 childRecords = CLng(cmd.QuickSelectSingleValue(sql, id))
 If childRecords > 0 Then
 MsgBox StringFormat("This CustomerGroup has {0} customer(s) associated to it and cannot be deleted.", childRecords), vbExclamation, "FK Constraint violation!"
 Exit Sub
 End If
 If MsgBox(StringFormat("Delete CustomerGroup #{0}?\n(this cannot be undone!)", id), vbExclamation + vbYesNo, "Please confirm") = vbNo Then Exit Sub
 sql = "DELETE FROM Planning.CustomerGroups WHERE Id = ?;"
 Dim success As Boolean
 success = cmd.QuickExecuteNonQuery(sql, id)
 If Not success Then MsgBox "Delete operation failed!", vbExclamation, "Warning"
 RefreshCustomerGroups
End Sub
Private Sub vwCustomerGroups_EditCustomerGroupCommand(ByVal id As Long, ByVal description As String)
 Dim newDescription As String
 newDescription = InputBox(StringFormat("Please enter a new description for CustomerGroup ID#{0}:", id), "Edit", description)
 If StrPtr(newDescription) = 0 Then Exit Sub
 Dim sql As String
 sql = "UPDATE Planning.CustomerGroups SET Description = ?, DateUpdated = ? WHERE Id = ?;"
 Dim success As Boolean
 success = cmd.QuickExecuteNonQuery(sql, newDescription, Now, id)
 If Not success Then MsgBox "Update operation failed!", vbExclamation, "Warning"
 RefreshCustomerGroups
End Sub

I feel like I could move the SQL outside the Presenter class and into some data service class.. but would that be overkill?

Besides the missing error-handling, what's there to say about this code?

• Mathieu Guindon: Nevermind the View() function, I forgot to remove it... it's just a relic from a previous approach / dead code now.
• Mathieu Guindon: It should also be noted that the MySQL backend somehow doesn't support FK's.
• Mathieu Guindon: Also SqlResultRow implements an IStringRepresentable interface that exposes a ToString method - should I not declare group as IStringRepresentable?

Answer 1

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