RFC: Modify ZigBeeNodeListener

[cdjackson]

Currently the ZigBeeNodeListener interface contains method for nodeAdded, nodeUpdated and nodeRemoved. It has been proposed in #649 to add a new nodeDiscovered method to be called when node discovery information changes.

I'm a little wary of adding new methods each time we want to notify of something new, and instead propose to change the interface to have a single method, and an enumeration stating the reason for the notification.

eg -:

nodeListener(ZigBeeNodeListenerNotification notification, ZigBeeNode node);

ZigBeeNodeListenerNotification {
    /*
     Node has been added to the network
     */
    ADDED,

    /*
     Node discovery data has been updated. This includes new endpoints or clusters being added to the node.
     */
    DISCOVERED,

    /*
     Node has been updated. This includes any changes to information such as routes and neighbours.
     */
    UPDATED,

    /*
     Node has been removed from the network
     */
    REMOVED
}

I do welcome comments on this approach versus the option of simply adding new methods.

[triller-telekom]

Sorry for the late reply.

To me this sounds like a reasonable approach. Plus point of this approach is that we have to break the API now once and afterwards we can simply extend this enumeration to introduce further reasons for notifications.

[mikomarrache]

Not having to rely on enums is generally a better approach. Enums introduce all sort of problems (client code may rely on ordinals, enum names, etc) while we don't get such issues using methods. For example, if I store the events triggered by this listener in a database, I would use the ordinal or the enum name. If enum order or entries names change, that would break client code (though client code still compiles).

Also, adding a method to an interface doesn't break the API as long as the method is declared as default (in this case with a no-op implementation).

I would go with the new method approach.

[cdjackson]

We already rely on enums in other listeners such as the network state listener, and there is quite extensive use of enums throughout the framework. If we never used enums for notifications, we'd have a lot of methods so the problem is it's not scalable IMHO.

If a user is importing the framework, then the enum is available, and it's generally not recommended to use the ordinal on enums in any case as this can always change - again this would be the same if you use the current enums.

[mikomarrache]

We already rely on enums in other listeners such as the network state listener, and there is quite extensive use of enums throughout the framework. If we never used enums for notifications, we'd have a lot of methods so the problem is it's not scalable IMHO.

Indeed enums are used throughout the framework but I was thinking about possible direction change for new or refactored code.
I'm not sure what you mean by scalability here? In terms of scalability, adding enum entries or default methods is similar.

If a user is importing the framework, then the enum is available, and it's generally not recommended to use the ordinal on enums in any case as this can always change - again this would be the same if you use the current enums.

Indeed, it's not recommended to use the ordinal on enums, but then you are left with the enum names. And you get exactly the same issue, changing the name of an enum entry may break existing code that still compiles. That's why you often see code that defines another "code" property at the enum level which never changes. This way enum entries order can be changed (i.e. ordinal changes) or renamed, but the code is left untouched.

Another point to take into account is that you may want to provide all sort of information that may depend on the type of event. Using methods, every listener method may have a different signature. With enums, that is not the case.

I just got into this ticket and I thought that would be good to give my two cent for this wonderful project.

[cdjackson]

I'm not sure what you mean by scalability here?

I mean if each listener had 5 or 10 different notifications, then you end up with a lot of methods being implemented.

changing the name of an enum entry may break existing code that still compiles.

Sure - but that's the same with changing method names. In general we try and avoid breaking changes to any external API such as changing enum names, or method names. I don't see that this is any different either way.

Another point to take into account is that you may want to provide all sort of information that may depend on the type of event.

Fair point, but that's not what we're really talking about. We have listeners that notify about one thing - eg the network state, or the node state. They pass information about that event - we're not using a single callback to provide very different information.

I just got into this ticket and I thought that would be good to give my two cent for this wonderful project.

Of course, and it's absolutely appreciated - it's good to have a debate which is why I try and raise these sort of issues here. Often I just "talk to myself" but getting other views is definitely useful and appreciated 👍