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

1 Answer 1

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

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.