Value Mapper service

I have some data I'm getting from an external source that I don't control and have been told that I have to map some of it to "user friendly" display values. So I wrote a little utility service to do this.

Populating selectors, I need to convert a list of values. Displaying data records, I have to convert individual values. And some values are obsolete and not shown in some circumstances. The records are stored in a database so that I can update the mapping at need.

I'm still learning how to write better Scala code, so comments on my use of idioms, error handling, and such would be especially helpful.

`/**
* This service class maps data values from an external source to
* more user friendly display values, or hides them altogether (use = false).
*/
class ValueMapper(context: EFLContext, data: Node) {
val logger = context.findService(classOf[LoggerFactoryService]) match {
case None => null
case Some(loggerFactory) => loggerFactory.getLogger(getClass.getName)
}

val sourceName = data.attribute("source") match {
case None => throw new InvalidParameterException("Definition must include a 'source' attribute.'")
case Some(a) => a.head.toString()
}

val source = context.findService(sourceName, classOf[DBSessionFactoryService]) match {
case None => throw new InvalidParameterException("Context does not contain a source DB with the specified name. (" + sourceName + ")")
case Some(db) => db
}

val dataCache = new mutable.HashMap[String, Map[String, MappedValue]]

def get(typeID: String, key: String, ignoreUnused: Boolean = true): Option[String] = {
logger.trace("[get] looking for: {} in {}", key, typeID, "")
var result: Option[String] = None

for (
data throw new IllegalArgumentException("Invalid type ID '" + typeID + "'.")
case Some(data) =>
strings.flatMap((key) => data.get(key).flatMap((record) => if (ignoreUnused && record.getUse == "n") None else Some(record.getLabel)))
}
}

private def

First of all, I'd change usage of Option[T].

Even standard documentation states:


The most idiomatic way to use an scala.Option instance is to treat it as a collection or monad and use map,flatMap, filter, or foreach

So I'd change logger instance like so:

val logger = context.findService(classOf[LoggerFactoryService]).
                map(_.getLogger(getClass.getName)).
                orNull

sourceName with:

val sourceName = data.attribute("source") map {
  _.head.toString()
} getOrElse {
  throw new InvalidParameterException("Definition must include a 'source' attribute.'")
}

And source just like that:

val source = context.findService(sourceName, classOf[DBSessionFactoryService]) getOrElse {
  throw new InvalidParameterException("Context does not contain a source DB with the specified name. (" + sourceName + ")")
}

Similar it's necessary to do for dataCache.get(typeID) match ... and for defaultContext.findService(classOf[ValueMapper]) match ... (both of them with combination map + getOrElse).

cacheGet(typeID) match ... is below.

More information you may find here.

As next step, you need to check for comprehenstion carefully.
Because 2 times you can improve readability of your code significantly.

F.e. in method get you can strip usage of variable in favour of constant and use if condition in the for statement:

def get(typeID: String, key: String, ignoreUnused: Boolean = true): Option[String] = {
  logger.trace("[get] looking for: {} in {}", key, typeID, "")

  val result = for {
    data <- cacheGet(typeID);
    record <- data.get(key) if record.getUse != "n" || !ignoreUnused
  } yield record.getLabel

  result
}

As opposite of this, you have very long chain in method map:

strings.flatMap((key) => data.get(key).flatMap((record) => if (ignoreUnused && record.getUse == "n") None else Some(record.getLabel)))

This is a perfect case to improve readability with for, as for is just a shorthand for map, flatMap and withFilter combinations.
And with knowledge of getOrElse we get the following:

def map(typeID: String, strings: List[String], ignoreUnused: Boolean = true): List[String] = {
  logger.trace("[map] looking for: {}", typeID)

  val result = for {
    data <- cacheGet(typeID)
    key <- strings
    record <- data.get(key) if (ignoreUnused && record.getUse == "n")
  } yield record.getLabel

  result getOrElse {
    throw new IllegalArgumentException("Invalid type ID '" + typeID + "'.")
  }
}

Also I'd like to add couple notes:

-
try to avoid usage of return keyword in Scala, because in most cases it's implemented with throwing exception and catching it, which has own overhead; I'd replace if (rawData == null || rawData.length == 0) return None with

if (rawData == null || rawData.length == 0) None
else {
   //code
}

-
if you need to have an error handling "scala-style", then you could use Try monadic construction; more information about it you can find here

StackExchange Code Review Q#67046, answer score: 5