Decoupling Presenter from "child" Repository

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

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.

• You could simply raise the custom errors I've mentioned and be done with it. (Easiest, but not necessarily the right thing to do.)

• 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.)

• 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.

StackExchange Code Review Q#60595, answer score: 2