Vortex: add push notification multi dispatcher interface

Review Request #438 — Created May 17, 2016 and submitted

tardypad
Lunr
push_notification_dispatcher_support_multicast
lunr

Add the missing unit tests declarations

unit tests

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
pprkut
  1. 
      
  2. generally we try to keep boolean methods in a scheme where they start with "is_". So maybe something like "is_multicast_capable()".
    
    At the very least however it should be "supports_multicast()".
    1. Coming back on this, even better would be if we just make it an interface that supporting classes can implement. Without any functions, just so that we could do something like "if ($foo instanceof MultiDispatcher)"

    2. I don't see the benefit of doing "if ($foo instanceof MultiDispatcher)" over "if ($foo->is_multicast_capable)"

      The only reason why interfaces might be better here would be if I create 2 of them with functions as such:
      - interface SingleDispatcher with functions push(), set_endpoint($endpoint), set_payload(&$payload)
      - interface MultiDispatcher with functions push(), set_endpoints($endpoints), set_payload(&$payload)

      But then I would better make the same for the response interface with one having function get_status() and the other one get_statuses() (or get_status($endpoint))

    3. I don't get your example. The main point here is that we want to mark certain objects as being multicast capable. Nothing more.

      Option 1) Force all objects to implement a is_multicast_capable() method that returns a constant TRUE or FALSE value. For this you need to add the method to an interface and all classes need to implement that code.
      8 LOC new per class.

      Option 2) Require all multicast object to implement a new MultiDispatcher interface (which is empty). Now all relevant classes just need to implement that interface.
      1 LOC changed, no new LOC

      I don't see how Option 2 does not win here.

    4. Well it just looks hacky to me to create an empty interface just for dispatchers with multicast.
      Especially that this interface will always stay empty.
      It doesn't provide any contract and is just used a flag.

      I personally prefer to have things more uniform across all the dispatcher classes
      It's simpler to understand its purpose when looking at the whole module
      It makes it as well more straighforward to use them: you won't be able to forget that specificity when adding a new dispatcher (since you have to implement that is_multicast_capable function)

    5. It's not hacky. It's a common design pattern: https://en.wikipedia.org/wiki/Marker_interface_pattern

      The only (reasonable) negative comment I found about it is that it implements functionality the class shouldn't know about (i.e. the class knows how it's going to be used). But same holds true for a function returning a constant.

      The only way I see to make this less hacky is to have separate methods for pushing individually and pushing in batch, and then using method_exists in the client code. But really, generally, Marker Interfaces are used to solve this problem.

  3. 
      
tardypad
tardypad
pprkut
  1. Ship It!
  2. 
      
tardypad
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...