search

fhircast / sandbox

FHIRcast Sandbox
GNU Affero General Public License v3.0
12 stars 3 forks source link

Consolidate Subscription Classes #56

Closed wmaethner closed 5 years ago

wmaethner commented 5 years ago

Hey @lbergnehr and @wdvr, would you two be able to take a first pass at this pull request which aims at fixing issue #53. The changes are pretty substantial and I am not completely finished, but would like to get some approval of the general direction before going much further.

The basic idea is that the Subscriptions class had 6 different class implementations mostly because the Subscription object has different properties depending on where we are at in the workflow. This was pretty confusing I think so I wanted to consolidate that. I put all the properties into a single Subscription class with the plan of performing the model validation ourselves rather than relying on the framework. We can define the different cases and what properties are required for them. Also for the JSON serialization (which also differed in the classes) I was hoping to use something like this https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Serialization_IContractResolver.htm.

Let me know what you think and if you see any glaring issues with this approach.

lbergnehr commented 5 years ago

Great initiative, @wmaethner! It does get a little confusing with a lot of intermediary abstract classes there just to get reuse in some small parts of the subclasses. What I do think is still important is to keep the conceptually different parts as different models in some way. For example, we still have a subscription and a subscription verification. How do we model these? They have some fields in common, and just copying that information into two classes might not be too bad after all; or just having a reference to a subscription from a verification could work.

The other way would be to find common parts and break that out into helper classes and compose the concrete models with the help of these. It wouldn't get the flat structure like it has now, so serialization would have to be customized to fit it. For example, SubscriptionIdentifier could be one of these helper classes that contains e.g. Id, Topic and other identifier fields.

wdvr commented 5 years ago

My opinion / personal preference: Since there is only one meaning for subscription, there should be only one subscription class*.

All other things that need a subcsription can just use composition, so i.e.

class subscriptionVerification(){
    Subscription sub;
    string challenge;
    ...
}

I think that makes it more clear for a reader of the code to see what the classes are all about.

Subscriptions can still have nullable/optional fields (e.g. reason for failure).

Separating serialization from the class definition sounds nice to me.