Improve encapsulation in ZigBeeNode

[nicolaiw]

/FYI @spudwebb

As nobody is prevented from accessing the Endpoints Property from the ZigBeeNode class as it is public the GetEndpoint(byte) method seems to be at most a helper method. I would recommend to make the Endpoints Property private and provide some more methodes to access the Endpoints. Maybe we could do something like the following to make it not breake existing application:

Endpoints => GetEndpoints(); // New method which might return a copy of the Endpoints.

What do you think?

[spudwebb]

sounds good you can make GetEndpoints() just return Endpoints.Values, it's fine for me.

[nicolaiw]

Would it be ok for you if the Endpoints property would return a copy of the list? I would recommend to not return the same enumeration used internaly by the ZigBeeNode class.

[spudwebb]

what list? Endpoints is a dictionary it is ok for me if GetEndpoints() returns Endpoints.Values or a copy of that collection if you prefer. I don't even need a public Endpoints property.

[Mr-Markus]

Yes, internally it is a dictionary. What about a ReadOnlyCollection Endpoints property with no set, that returns a copy instead?

[nicolaiw]

My suggestion: make the Endpoints Dictionary private and provide a GetEndpoints() methode which returns a ReadOnlyCollection or what ever.

[Mr-Markus]

Don't forget that Endpoints need to be accessible for DataStore with GetDao() and SetDao() methods on ZigBeeNode like here:

https://github.com/Mr-Markus/ZigbeeNet/blob/76472a6b91931cdb960dc7f7311a88f631a7b27a/libraries/ZigBeeNet/ZigBeeNode.cs#L608-L613

[nicolaiw]

Will send PR ... @Mr-Markus It will as this happens in the ZigBeeNode class or am I missing your point?

[Mr-Markus]

Oh yes. It is in same class and private endpoints member will be availible there^^

[Mr-Markus]

I have added a public property named Endpoints, like it was before which resturns a copy of private _endpoints as a ReadOnlyDictionary for compatibility reasons.

I used this property in my applications and had to replace it with GetEndpoints() after last changes. I think that it is an important public property and to be honest, in my opinion properties are more C# than Get() methods

But don't worry. Your method is still there :-)

[Mr-Markus]

@nicolaiw Are there any other properties that should be refactored like this to encapuslate it's data?

[nicolaiw]

Just in case of the ZigBeeNode class (I did not look at the other classes) .. that would still be up for discussion. As all the HashSets might manipulated by the ZigBeeNode class (see for example UpdateNode(ZigBeeNode)) it might be a good idea to adjust the encapsulation for these HashSets as well.

[nicolaiw]

This also might be wrongly adopted from our reference java project:

https://github.com/zsmartsystems/com.zsmartsystems.zigbee/blob/master/com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/ZigBeeNode.java

The Sets are private and the getters are synchronized and returns new Sets.

[Mr-Markus]

Thx for these PRs. I will close this issue now and you could another if there are more improvements