Closed alechenninger closed 8 years ago
Overall, I like approach generally; my only hesitation is that some of these interfaces are super generic (like the Future-esque one) and can perhaps be replaced by standard ones. I think changes are good once we get those interfaces down right.
Thank you very much sir! I'll mess around with implementing Future, error handling, and see how it goes
@kahowell Used Future, was easier than I thought (at least for our simple scenario). Let me know what ya think. Thanks!
Full disclosure: not intending to complete the error handling all right now, wanna do some TDD to complete that stuff. More wanna get your stamp of approval on design and names, work on the nitty gritty next (which involves first testing the non-error routes actually work).
I've started implementing some of the actual meat of this in another branch FYI. Leaving this as a way to finishing talking about high level details before piling implementation on top of it.
Some 'TODOs' left here, but there are a lot of those everywhere really right now.
Okay, first why this refactoring?
We have two places where data lookup may happen. The first is when going from Notifications -> DocumentEvents, you may need to do some lookups based on business logic if necessary information can not and should not be captured at time of the notification. The second is obviously when going from DocumentEvents to documents, you need to lookup the state of the thing(s) used to build that document.
Currently, I had gone down two different ways of doing those things. The first was with Notifications, which had the API on them directly (
toDocumentEvents
). I like that because this captures where that business logic should naturally live; it doesn't push it off to a 'NotificationHandler' which would necessarily have all the knowledge of all the things. Actually, there is another way you could do that, which is have the "handler" call back to internal APIs on notifications to keep logic on the 'notification' instance side of the wall, but this is a little awkward. This is actually way number 2, and what I did with DocumentRepository and DocumentEvents, however this is only apparent if you look at the lightblue implementation since it is up to implementation to follow this pattern.The only plus side to the "repository" approach is that you can do bulk requests more easily.
So I felt this was an opportunity to unify these two approaches and this is what I came up with.
It's basically a combination approach. Notifications and DocumentEvents both have APIs on them that return a Result object. Notifications
toDocumentEvents
returns a Result of DocumentEvents. DocumentEventslookupDocument
returns a Result of some object (its type may be different based on impl).Because Result is an interface, it can do its lookups lazily. This allows us to build many Result objects (each in O(1)) without actually doing any remote calls, and then do all the remote calls once we ask for any one Result for something (like the value it contains within, or if there were any errors, etc.).
This is best of both worlds IMO with some cost of complexity when doing the bulk / queued requests, but not much compared to the old bulk request approach IMHO, which had a different kind of complexity.
You can see an example of how this works when writing DocumentEvent and Notification implementations in our internal business logic repo (branch consolidate-results-and-bulk-ops).
What's left