5
\$\begingroup\$

Still pursuing the white rabbit, I had an IPresenter interface implementation featuring this method:

Private Sub ExecuteAddCommand()

    Dim orderNumber As String
    If Not RequestUserInput(prompt:=GetResourceString("PromptExcludedOrderNumberMessageText"), _
                            title:=GetResourceString("AddPromptTitle"), _
                            outResult:=orderNumber, _
                            default:=0) _
    Then
        Exit Sub
    End If

    'tight coupling...
    Dim orderRepo As IRepository
    Set orderRepo = New OrderHeaderRepository

    Dim values As New Dictionary
    values.Add "number", orderNumber

    Dim orderModel As New SqlResult
    orderModel.AddFieldName "Number"

    Dim order As SqlResultRow
    Set order = orderRepo.NewItem(orderModel, values)

    Dim orderId As Long
    orderId = orderRepo.FindId(order)

    If orderId = 0 Then
        MsgBox StringFormat(GetResourceString("InvalidOrderNumberMessageText"), orderNumber), _
               vbExclamation, _
               GetResourceString("InvalidOrderNumberTitle")
        Exit Sub
    End If

    Dim reason As String
    If Not RequestUserInput(prompt:=GetResourceString("AddExcludedOrderMessageText"), _
                            title:=GetResourceString("AddPromptTitle"), _
                            outResult:=reason, _
                            default:=GetResourceString("DefaultExcludedOrderReason")) _
    Then
        Exit Sub
    End If

    Repository.Add NewExcludedOrder(orderHeaderId:=orderId, reason:=reason)
    IPresenter_ExecuteCommand RefreshCommand

End Sub

The problem is that, out of 12 implementations so far, it was the only one with such tight coupling - for every single other case, it made sense to implement a "child" presenter with its own repository.

The tightly coupled code works, but can't be tested offline, which was the entire point/purpose of going down the rabbit hole with implementing MVP and Repository patterns in VBA. I had to do something about it.


So I added a FindCommand to the CommandType enum, and implemented an OrderHeaderPresenter:

Option Explicit

Private Type tPresenter
    MasterId As Long
    Repository As IRepository
    DetailsPresenter As IPresenter
    View As IView
End Type

Private this As tPresenter
Implements IPresenter

Private Function NewOrderHeader(Optional ByVal id As Long = 0, Optional ByVal number As String = vbNullString) As SqlResultRow

    Dim result As SqlResultRow

    Dim values As New Dictionary
    values.Add "id", id
    values.Add "number", number

    Dim orderModel As New SqlResult
    orderModel.AddFieldName "id"
    orderModel.AddFieldName "number"

    Set result = Repository.NewItem(orderModel, values)
    Set NewOrderHeader = result

End Function

Public Property Get Repository() As IRepository
    Set Repository = this.Repository
End Property

Public Property Set Repository(ByVal value As IRepository)
    Set this.Repository = value
End Property

Private Function IPresenter_CanExecuteCommand(ByVal commandId As CommandType) As Boolean
    If commandId = FindCommand Then IPresenter_CanExecuteCommand = True
End Function

Private Property Set IPresenter_DetailsPresenter(ByVal value As IPresenter)
'not implemented
End Property

Private Property Get IPresenter_DetailsPresenter() As IPresenter
'not implemented
End Property

Private Function IPresenter_ExecuteCommand(ByVal commandId As CommandType) As Variant

    Select Case commandId

        Case CommandType.FindCommand
            IPresenter_ExecuteCommand = ExecuteFindCommand

        Case Else
            'not implemented

    End Select

End Function

Private Function ExecuteFindCommand() As Long

    Dim orderNumber As String
    If Not RequestUserInput(prompt:=GetResourceString("PromptExcludedOrderNumberMessageText"), _
                            title:=GetResourceString("AddPromptTitle"), _
                            outResult:=orderNumber, _
                            default:=0) _
    Then
        Exit Function
    End If

    Dim order As SqlResultRow
    Set order = NewOrderHeader(number:=orderNumber)

    Dim orderId As Long
    orderId = Repository.FindId(order)

    If orderId = 0 Then
        MsgBox StringFormat(GetResourceString("InvalidOrderNumberMessageText"), orderNumber), _
               vbExclamation, _
               GetResourceString("InvalidOrderNumberTitle")
        Exit Function
    End If

    ExecuteFindCommand = orderId

End Function

Private Property Let IPresenter_MasterId(ByVal value As Long)
    this.MasterId = value
End Property

Private Property Get IPresenter_MasterId() As Long
    IPresenter_MasterId = this.MasterId
End Property

Private Property Set IPresenter_Repository(ByVal value As IRepository)
    Set Repository = value
End Property

Private Property Get IPresenter_Repository() As IRepository
    Set IPresenter_Repository = Repository
End Property

Private Sub IPresenter_Show()
'not implemented
End Sub

Private Property Set IPresenter_View(ByVal value As IView)
'not implemented
End Property

Private Property Get IPresenter_View() As IView
'not implemented
End Property

For this to work, I had to change IPresenter.ExecuteCommand from a Sub to a Function returning a Variant, so that commands like Find and Search can return an object or a value.

And that allowed me to refactor my coupled presenter method to this:

Private Sub ExecuteAddCommand()

    Dim orderId As Long
    orderId = this.DetailsPresenter.ExecuteCommand(FindCommand)

    If orderId = 0 Then Exit Sub

    Dim reason As String
    If Not RequestUserInput(prompt:=GetResourceString("AddExcludedOrderMessageText"), _
                            title:=GetResourceString("AddPromptTitle"), _
                            outResult:=reason, _
                            default:=GetResourceString("DefaultExcludedOrderReason")) _
    Then
        Exit Sub
    End If

    Repository.Add NewExcludedOrder(orderHeaderId:=orderId, reason:=reason)
    IPresenter_ExecuteCommand RefreshCommand

End Sub

Here's the OrderHeaderRepository implementation, which partly explains the holes in the presenter implementation:

Option Explicit
Private cmd As New SqlCommand
Private Const selectString As String = "SELECT Id, Number, OrderDate, OrderTypeId, SeasonId FROM Planning.OrderHeaders"
Implements IRepository

Private Sub IRepository_Add(ByVal value As SqlResultRow)
'not implemented
End Sub

Private Function IRepository_Count() As Long
    IRepository_Count = RepositoryImpl.Count("Planning.OrderHeaders")
End Function

Private Function IRepository_FindId(ByVal naturalKey As SqlResultRow) As Long

    Dim sql As String
    sql = "SELECT Id FROM Planning.OrderHeaders WHERE Number = ?;"

    Dim result As Long
    'todo: find out why query won't return a value with a string parameter.
    '      underlying db table column is a varchar, doesn't make sense to CLng() here.
    result = cmd.QuickSelectSingleValue(sql, CLng(naturalKey("number")))

    IRepository_FindId = IIf(IsEmpty(result), 0, result)

End Function

Private Function IRepository_GetAll() As SqlResult
'not implemented. too many results to efficiently process in a SqlResult.
End Function

Private Function IRepository_GetById(ByVal id As Long) As SqlResultRow
    Set IRepository_GetById = RepositoryImpl.GetById(selectString, id)
End Function

Private Function IRepository_NewItem(ByVal Model As SqlResult, ByVal values As Scripting.IDictionary) As SqlResultRow
    Set IRepository_NewItem = RepositoryImpl.NewItem(Model, values)
End Function

Private Sub IRepository_Remove(ByVal id As Long)
'not implemented
End Sub

Private Function IRepository_Search(ByVal terms As SqlResultRow) As SqlResult
'not implemented. possibly too many results to efficiently process with a SqlResult.
End Function

Private Sub IRepository_Update(ByVal id As Long, ByVal value As SqlResultRow)
'not implemented
End Sub

A few things annoy me slightly:

  • Most of both IPresenter and IRepository interfaces are left unimplemented for the OrderHeader model - the presenter is only exposing a Repository property... which somewhat smells.
  • The OrderHeaderPresenter, while physically decoupled from the ExcludedOrdersPresenter, is still functionally coupled with it - the FindCommand is prompting the user for an order number to be excluded, which is what needs to happen, but the OrderHeaderPresenter shouldn't be using resources meant to be used in another implementation. Or should I just change that string to read something more generic, like "Please enter an order number:"?

Anything else sticks out? Any recommendations?

\$\endgroup\$
2
\$\begingroup\$

Let's talk a little about your code the way it is right now. This happens a lot.

Private Property Set IPresenter_DetailsPresenter(ByVal value As IPresenter)
'not implemented
End Property

Private Property Get IPresenter_DetailsPresenter() As IPresenter
'not implemented
End Property

I like that you took the time to comment that it's not implemented, but anyone calling OrderHeaderPresenter.DetailsPresenter will have to dive into the source code to find out why it's not doing anything. Worse, it might take the unwary dev a long time to realize that it isn't doing anything. All of these should raise FeatureNotImplementedErrors.

Putting that aside, you're right. The simple fact that all of these are not implemented does smell, but we'll get back to that.


Private Function IPresenter_ExecuteCommand(ByVal commandId As CommandType) As Variant

   Select Case commandId

       Case CommandType.FindCommand
           IPresenter_ExecuteCommand = ExecuteFindCommand

       Case Else
            'not implemented

   End Select

End Function

The Else case here makes me think that OrderHeaderPresenter isn't really a IPresenter. It only implements one of the command types and you added that command type specifically for this implementation. At the very least, don't let it silently fail. Again, raise an error here.


Private Function ExecuteFindCommand() As Long

    Dim orderNumber As String
    If Not RequestUserInput(prompt:=GetResourceString("PromptExcludedOrderNumberMessageText"), _

Your execute find command excludes the order number it's given? How is that a "find" command. Find is for positive hits, not negatives. Granted, I don't have a better name for you, but think on it.

The order ID is added to some ExcludedOrders table.

Okay, that's unclear from solely reading the code. Add a comment here.


Private Sub IPresenter_Show()
'not implemented
End Sub

If that doesn't convince you that this isn't a IPresenter I don't know what will. It's a presenter that doesn't present anything.


A lot of the same with IRepository methods not being implemented.

So, that's an awful lot of "Raise errors" and "This isn't really a IPresenter", but not a lot of advice on how to fix it. In my mind, there aren't a lot of options.

  1. You could simply raise the custom errors I've mentioned and be done with it. (Easiest, but not necessarily the right thing to do.)
  2. Create two new interfaces. I think creating a IRepositoryBase that holds the common contracts makes sense. OrderDetailsHeader would implement only IRepositoryBase and all of your other existing classes would need to be changed to implement both IRepositoryBase and IRepository. The second new interface would be for those "presenter" methods that aren't really presenter methods. (By far the hardest, but perhaps the most "correct" option.)
  3. Create a single new interface for this new class. It will duplicate some code from both IRepository and IPresenter.

I would opt for option number three myself. I hope I've done enough to convince you that OrderDetailsPresenter is not really the same thing as an IPresenter. Making it it's own thing makes sense. This could also be considered the most "correct" thing to do because you really should never change an interface once it's been created and used.

To quote CPearson:

Once you have defined your interface class module, you must not change the interface. Do not add, change, or remove any procedures or properties. Doing so will break any class that implements the interface and all those classes will need to be modified. Once defined, the interface should be viewed as a contract with the outside world, because other objects depend on the form of the interface. If you modify an interface, any class or project that implements that interface will not compile and all the classes will need to be modified to reflect the changes in the interface. This is not good. In a large application, particularly one developed by more than one programmer, your interface may be used in places of which you are not aware. Changing the interface will break code in unexpected places. If you find the absolute need to change an interface, leave the original interface unchanged and create a new interface with the desired modifications. This way, new code can implement the new interface, but existing code will not be broken when using the original interface.

Emphasis mine.

\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent! One thing though: "Your execute find command excludes the order number it's given? How is that a "find" command." The feature is really prompting the user for an order number, and finding its ID; the ID is added to a table called something like ExcludedOrders. Sorry for the confusion :) \$\endgroup\$ – Mathieu Guindon Aug 20 '14 at 18:14
  • \$\begingroup\$ Noted and updated. \$\endgroup\$ – RubberDuck Aug 20 '14 at 18:15
  • \$\begingroup\$ About unimplemented interface members: it's how I work around the impossibility to extend an interface with another; in a truly OOP language I'd probably have an IPresenter and an IChildPresenter : IPresenter extending it with the MasterId, and a IMasterPresenter : IPresenter adding a DetailsPresenter member, but like IRepository and IRepositoryBase idea, it's not something that facilitates usage (I actually tried that in early design stages). I think the best option is a higher abstraction level, like make IPresenter need some IDataService instead of a IRepository. \$\endgroup\$ – Mathieu Guindon Aug 21 '14 at 0:12
  • 1
    \$\begingroup\$ Then IDataService can expose a List<IRepository> - and then I can get rid of the ill-designed OrderHeaderPresenter half-implementation... but yeah that means modifying a dozen existing implementations. I'm ok with that, if the result means cleaner code! ;) \$\endgroup\$ – Mathieu Guindon Aug 21 '14 at 0:18
  • 1
    \$\begingroup\$ In C#, yes. In VBA.... \$\endgroup\$ – Mathieu Guindon Aug 21 '14 at 0:24

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.