Decoupling Presenter from "child" Repository

Question

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?

Answer 1

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.

• Mathieu Guindon: 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 :)
• RubberDuck: Noted and updated.
• Mathieu Guindon: 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.
• Mathieu Guindon: 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! ;)
• Mathieu Guindon: In C#, yes. In VBA....