Disallow SDK consumers from using unsanitary methods

[hicklin]

Following this comment by @andreilitvin, the Service Area Delegate's methods where made public and the Service Area Instance class was removed from being a friend. This was done to get the PR over the line and merged, however, this change dose not solve the issue that the previous code was seeking to solve.

This issue has been raised to discuss whether we should revert the change made here or if we could find a better compromise that addresses the concerns of all engineers.

[andy31415]

I think the SDK classes should be designed in a way that they are useful as stand-alone. Anything that mandates friend over the fear of misuse is too tight of a coupling and should not be a pattern that we use.

[hicklin]

Our preferred way of implementing this cluster is such that the SDK will take care of managing all the attributes and their data. However, due to the complex nature of some of these attributes and the variety of targets that the SDK needs to work with, the storage of this data has to be delegated to the SDK consumer to allow them to implement what's best for their target.

Given this, we could have opted to delegate the responsibility of both storing the data and validating the data to the SDK consumer. This would have resulted in a Delegate class who's methods included the necessary sanity checks and could therefore be used by anyone. In this case, the Delegate methods would be public and the Instance class would not need to be a friend of the Delegate class. However, we thought that wherever possible, the SDK should take the responsibility of data validation.

Our solution to this was to perform data validation in the SDK via the Instance class and only delegate the data storage implementation to the SDK consumer via the Delegate class. This meant that the virtual methods in the Delegate class were going to be simple but unsanitary, as they did not required any data validation. Hence, to protect the SDK consumer form accidentally using these unsanitary methods, we opted to disallow their use by marking them as protected. This allows the SDK consumer's Delegate implementation to implement them, but disallows the SDK consumer's application form using them.

I accept the general principle of not coupling classes in this way wherever possible. But the Instance and Delegate classes are always coupled, at least a little bit, since they require access to each other. There are other patterns that we can use here, however, this pattern allows us to separate the functionality that the SDK consumer needs to implement in it's own class. Is there a pattern that you have in mind that you think would work better than this while achieving the same goals?

[hicklin]

I have a proposal to address the concerns raised here. @andy31415 & @johnfierke, can you let me know if you have any objections to this pattern?

Currently, the Delegate class contains all the methods that the SDK consumer is required to implement. This has caused there to be a tight coupling with the Instance class via the use for friend.

The list of methods that need to be implemented by the SDK consumer for the Service Area cluster can be split into 2 main groups; a "business logic" group that contains methods that may need to interact with other parts of the state machine, and a "memory management" group that store, get or modify data. The latter group can be further split into a group of "safe" methods, this includes all the Get... and Is... methods, and an "unsafe" group that contains all the Add..., Modify... and Clear... methods. All unsafe methods have a mirror Instance method that does the necessary sanity checking before the Delegate's unsafe method is called.

My proposal is to move all of the memory management related methods from the Delegate class to the Instance class. All the "safe" methods will be made public and all the "unsafe" methods will be made private. The names of the private methods well have to change not to clash with the "safe" methods publicly defined it the Instance class. e.g. AddSupportedMap to AddSupportedMapRaw.

This change would mean that the SDK consumer will now have to implement their own version of both the Instance and Delegate classes.

[andy31415]

Assuming the above proposal has no friend class/function in it, I am ok with it.

I am unsure if Instance vs Delegate is the ideal split (have not looked at it) since I wonder if instance does not increase in size a lot, however whatever seems clean. I am not oposing in a business vs memory management split (in fact that sounds like it would make code more maintainable)

[hicklin]

I have what I think is a better proposal. Instead of moving the memory management methods, that is all the GetXxx, IsXxx, AddXxx, ModifyXxx and ClearXxx methods, to the Instance class, we move them to a new "memory management" delegate class. All of these methods will be public but the Instance's class pointer to this delegate will be private so only the Instance can access this delegate. Unlike the current delegate, this one would be simpler and uncoupled to the Instance class as it would not even need to have a pointer back to it.