10
\$\begingroup\$

I am a moderately new Scala developer working mostly by myself, so I don't have anyone to tell me what I'm doing wrong (except that the system mostly does work, so it can't be too awful). I would appreciate some feedback on how to make better use of Scala as well as any comments about obvious performance issues.

The class below is part of our utility library. It's part of our OSGi-like infrastructure. I didn't use OSGi because the web application is hosted on a Tomcat server but I liked many of the ideas behind it, so I've incorporated them into my own library.

package edu.stsci.efl.ml

import scala.collection.mutable.{HashSet, Stack, HashMap}
import scala.xml.Node

import java.util.Date


/**
  *  Provides a container for services that are being provided by various modules.
  *
  *  The default context can be had from the ModuleLoader object.
  *
  *  Services are accessed by the class of the trait that they implement. For example,
  *  logging services implement LoggerFactoryService so to get access the service, call
  *  getService(classOf[LoggerFactoryService]) to find the service that was defined in
  *  the ModuleLoader configuration file.
  */
class EFLContext(val name: String) {

  private var modules: HashMap[String, EFLModule] = new HashMap[String, EFLModule]
  private val shutdownList: Stack[EFLModule] = new Stack[EFLModule]
  private val importList: Stack[EFLModule] = new Stack[EFLModule]
  private val services = new HashSet[Object]

  private var myDependencies: List[EFLContext] = null
  private var myDependents: List[EFLContext] = null

  /**
    * Used in testing to verify that the correct number of modules have been loaded.
    */
  def getModuleCount = modules.size

  /**
    * Use by the ModuleLoader to pass a module definition from the XML configuration file.
    */
  def load(moduleDefinition: Node) {

    val moduleName = (moduleDefinition \ "@name").toString()

    val className = (moduleDefinition \ "@class").toString()

    try {
      val rawClass = Class.forName(className)
      val moduleClass = rawClass.asSubclass(classOf[EFLModule])
      val instance = moduleClass.newInstance
      instance.start(this, moduleDefinition)

      shutdownList.push(instance)
      modules.put(moduleName, instance)
    }
    catch {
      case e: Exception => {
        println("Failed to initialize module. Error:")
        e.printStackTrace()
        throw new EFLInitializationFailure(moduleDefinition.toString(), e)
      }
    }
  }

  /**
    *
    */
  def resolveImport(data: Node) {
    val contextName = getAttribute(data, "context")
    val name = getAttribute(data, "name")
    val module = {
      val context = ModuleLoader.getContext(contextName)
      if (context == null) null
      else {
        val result = context.modules.getOrElse(name, null)
        if (result != null) addDependency(context)
        result
      }
    }

    if (module == null) throw new IllegalArgumentException("Unable to resolve import of module '" + name + "' in context '" +
                                                       contextName + "'.")

    module.addContext(this)
    importList.push(module)
    modules.put(name, module)
  }

  def getAttribute(data: Node, name: String): String = {
    val attribute = data.attribute(name)
    if (attribute.isEmpty) throw new IllegalArgumentException("Failed to specify attribute " + name + " in import.")

    attribute.head.toString()
  }

  /**
    * Used by modules at application startup to announce services that they are providing.
    */
  def addService(service: Object) {
    services += service
  }

  /**
   *
   */
  def addDependency(fLContext: EFLContext) {
    myDependencies match {
      case null => myDependencies = List[EFLContext](fLContext)
      case _ => myDependencies = myDependencies :+ fLContext
    }
    fLContext.addDependent(this)
  }

  /**
   *
   */
  def addDependent(fLContext: EFLContext) {
    myDependents match {
      case null => myDependents = List[EFLContext](fLContext)
      case _ => myDependents = myDependents :+ fLContext
    }
  }

  def hasDependents: Boolean = (myDependents != null)

  def removeDependency(context: EFLContext) {
    myDependencies = myDependencies.filterNot((c: EFLContext) => c == context)
    if (myDependencies.isEmpty) myDependencies = null

    context.removeDependent(this)
  }

  def removeDependent(context: EFLContext) {
    myDependents = myDependents.filterNot((c: EFLContext) => c == context)
    if (myDependents.isEmpty) myDependents = null
  }

  /**
    * Used by modules at application shutdown to terminate services.
    */
  def removeService(service: Object) = {
    services -= service
  }

  /**
    * Called by the ModuleLoader at application shutdown so that loaded modules can be stopped.
    */
  def shutdown() {
    if (modules == null) throw new RuntimeException("Attempt to shutdown context that was already shutdown.")
    if (hasDependents) throw new RuntimeException("Attempt to shutdown context with loaded dependent contexts.")

    while (!importList.isEmpty) {
      val next = importList.pop()
      next.removeContext(this)
    }

    while (!shutdownList.isEmpty) {
      val next = shutdownList.pop()
      next.stop(this)
    }

    modules.clear()
    if (services.size > 0) {
      println("Improper cleanup: " + services.size + " services still registered.")
      services.foreach((s) => println(s))
    }
    modules = null

    while (myDependencies != null) {
      val next = myDependencies.head
      removeDependency(next)
    }
  }

  /** Search for a given service by the class/trait the service implements/extends.
    *
    * @param serviceClass The class of a service that is needed.
    * @return Option[A] 'Some' of the given type, or 'None' if no service object extending that type is registered.
    */
  def findService[A](serviceClass: Class[A]): Option[A] = {
    services.find(serviceClass.isInstance).asInstanceOf[Option[A]]
  }

  /** Search for a given service by the class/trait the service implements/extends.
    *
    * @return Service of the given type, or null if no service object extending that type is registered.
    */
  @Deprecated
  def getService[A](serviceClass: Class[A]): A = {

    val temp = services.find(serviceClass.isInstance)

    if (temp == None) {
      null.asInstanceOf[A]
    }
    else {
      temp.get.asInstanceOf[A]
    }
  }
}
\$\endgroup\$
10
\$\begingroup\$

Some words at beginning: I have never used OSGI, so I can't say if the tips below may all work in your context. If they do not, than keep them in the back of your head and use them when you can.


One of the most important rules is to avoid mutable state if possible. This will result in more type-safe and easier to read code. This is not always possible but most times it is.


Never use null if you don't have to use it. Use Option type instead or use empty collections.

private var myDependencies: List[EFLContext] = null
// =>
private var myDependencies: List[EFLContext] = Nil

Now you don't have to pattern match against null any more:

def addDependency(fLContext: EFLContext) {
  myDependencies match {
    case null => myDependencies = List[EFLContext](fLContext)
    case _ => myDependencies = myDependencies :+ fLContext
  }
  fLContext.addDependent(this)
}
// =>
def addDependency(fLContext: EFLContext) {
  myDependencies +:= flContext
  fLContext addDependent this
}

Here, never append to a List, this is inefficient (O(n)). Instead use prepending (O(1)). If you have to prepend elements, than use an IndexedSeq. If you want you can use operator notation.


In Scala everything returns a value, thus there is no need to mark a method that it returns a value:

def getModuleCount = modules.size
// =>
def moduleCount = modules.size

method -= is deprecated. Use filterNot instead.


Scala has some nice methods defined in Predef. One of them can check input params:

if (modules == null) throw new RuntimeException("Attempt to shutdown context that was already shutdown.")
// =>
require(modules.nonEmpty, "Attempt to shutdown context that was already shutdown.")

Don't traverse through the elements of a collection with a while-loop. Instead use higher-order functions (foreach instead of while-loop). Furthermore in Scala there is no need to explicitly use a Stack. A List is already a Stack (when you prepend elements what you should do)!

while (!importList.isEmpty) {
  val next = importList.pop()
  next.removeContext(this)
}
// =>
importList foreach { _ removeContext this }
importList = Nil

Pattern match Options:

if (opt.isDefined) ...
else opt.get ...

opt match {
  case None => ...
  case Some(o) => o ...
}

Use homogeneous types in collections and not Object if possible.

private val services = new HashSet[Object]
// =>
val services1 = new HashSet[Type1]
val servicesN = new HashSet[TypeN]

If you do so there is no need for casts any more like in your method findService. Of course you should choose better names for the variables than I did.


To easily come from null to Option you can use its apply method:

Option(null) // => None
Option(non_null_value) // => Some(non_null_value)

No need to use curly brackets in a case-block:

case _ => {
  a
  b
}
// =>
case _ =>
  a
  b

You can use Option in method resolveImport:

def resolveImport(data: Node) {
  val contextName = getAttribute(data, "context")
  val name = getAttribute(data, "name")
  val context = Option(ModuleLoader.getContext(contextName))
  val result = context flatMap { _.modules get name }
  result match {
    case None =>
      throw new IllegalArgumentException("Unable to resolve import of module '"+name+"' in context '"+contextName+"'.")  
    case Some(module) =>
      this addDependency module
      module addContext this
      importList push module
      modules += name -> module
  }
}
\$\endgroup\$
  • \$\begingroup\$ Lots of good stuff! The comment about services won't work because no two services are of the same type - a context is sort of a phone directory where objects that provide a given service can be found by objects that need that particular service. I already knew that curly braces were not needed in case blocks - as a matter of personal taste I find adding them makes the structure clearer. Thank you! \$\endgroup\$ – Donald.McLean Mar 16 '12 at 13:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.