13
\$\begingroup\$

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?

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

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
      
\$\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.