Event dispatcher

In the last 2 years or so, I've learned a great deal about how to write better Scala code, but I know that I've barely scratched the surface. This is part of a library that I've been using. It has evolved a great deal since the first version and I'm curious to see how many mistakes I'm still making. I am particularly curious about what I can do to make better use of the Scala language.

This is already fairly long, so I'm leaving out my test class, though I would be happy to see test case suggestions.

EventDispatch.scala

package edu.stsci.efl.services

import edu.stsci.efl.events.EFLEvent
import net.liftweb.actor.LiftActor

/**
 * This is the interface definition for classes that provide application event services.
 */
trait EventDispatchService {
  def registerForEvent(name: String, callback: (EFLEvent) => Unit): EventRegistration
  def registerConditionallyForEvent(name: String, callback: (EFLEvent) => Unit, condition: (String, Any)): EventRegistration
  def registerActorForEvent(name: String, actor: LiftActor): EventRegistration
  def postEvent(event: EFLEvent)
  def postDelayedEvent(event: EFLEvent, delay: Int)
}

trait EventRegistration {

  private var status: EventRegistrationStatus = ERSPending
  def deregister()
  def is (s: EventRegistrationStatus) = s == status
  def update(s: EventRegistrationStatus) {status = s}
  def isAcceptingEvents = status == ERSActive
}

case class EventRegistrationStatus(name: String)

object ERSPending extends EventRegistrationStatus("pending")
object ERSActive extends EventRegistrationStatus("active")
object ERSPaused extends EventRegistrationStatus("paused")
object ERSTerminating extends EventRegistrationStatus("terminating")
object ERSTerminal extends EventRegistrationStatus("terminal")

Event.scala

```
package edu.stsci.efl.events

import edu.stsci.efl.ml.EFLContext

import org.slf4j.Logger

import scala.collection.mutable
import scala.xml.Node

/**
* A generic event class with a name and named properties.
*/

Your whole design looks very mutable, but because I don't know your domain or the libraries you use I can't say to what extend it makes sense to functionalize it. Some thoughts while looking through the code:

Always specify return types for public members (even consider Unit), especially in an interface definition:
def postEvent(event: EFLEvent) → def postEvent(event: EFLEvent): Unit. This makes it easier to understand someone else code (sometimes the inferred types are nontrivial).

The return types in subclasses need to be specified most of the time too (what you did). Scala allows covariant return types in override definitions which can be a problem because type inference makes use of this feature and therefore can result in compilation errors in some special cases where one only works with concrete subclasses and not the interfaces.

Never initialize an instance field with a default value, always use the underscore:
var context: EFLContext = null → var context: EFLContext = _. They have a different semantic meaning! The latter one leaves the initialization to the JVM, which does the thing one expects. The former one will initialize the field with null in the constructor, the JVM initialization is done before the constructor is even called. For subclass relationships this can make a huge difference:

scala> trait A { var x: String; init(); def init(): Unit = x = "initialized" }
defined trait A

scala> class B extends A { override var x: String = null }
defined class B

scala> class C extends A { override var x: String = _ }
defined class C

scala> (new B).x
res0: String = null

scala> (new C).x
res1: String = initialized

xs.foreach(n => {}) can be written as xs.foreach { n => }

I'm not familiar with Lift and therefore don't know their preferred coding style but when I see def registerForEvent(name: String, callback: (EFLEvent) => Unit): EventRegistration and an actor I can only think about callback hell. When I have actors then I prefer having only actors that send each other messages. Maybe a callback free and actor only solution makes sense for you too, think about it.

Your try-catch blocks look cumbersome. They can be abstracted away with something like

def withErrorLogging(message: String)(f: => Unit) =
  try f catch { case t: Throwable => logger.error(message, t) }

withErrorLogging("[registerActorForEvent]") { ... }

In fact, all of your register/deregister functions look the same. Try to move out the part which differ (which seems to be only the code in the try-block)

Sometimes you use a Thread.sleep(n), which shouldn't be used in an actor environment, because they block.

I would never pass a mutable HashSet: def evaluate(..., callbacks: mutable.HashSet[ConditionalRegistration]). Do you know that it is not changed in this function, or is it intended to be changed by this function? In any case always prefer immutability to the outside, it makes it a lot easier to reason about control flow.

StackExchange Code Review Q#48210, answer score: 5