Value Mapper service

Question

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 <- cacheGet(typeID);
      record <- data.get(key)
    ) {
      if (record.getUse != "n" || !ignoreUnused) result = Some(record.getLabel)
    }

    result
  }

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

    cacheGet(typeID) match {
      case None => 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 cacheGet(typeID: String): Option[Map[String, MappedValue]] = {
    logger.trace("[cacheGet] looking for: {}", typeID)

    dataCache.get(typeID) match {
      case Some(m) =>
        logger.trace("[cacheGet] already in cache.")
        Some(m)
      case None =>
        val session = source.createSession
        val queryData = Map[String, AnyRef]("mapType" -> typeID)
        val rawData = session.getUsingNamedQuery("findMappedValueByType", queryData)

        if (rawData == null || rawData.length == 0) return None

        val result = rawData.map{
          case v: MappedValue => (v.getRefID, v)
        }.toMap

        session.close()
        dataCache.put(typeID, result)
        logger.trace("[cacheGet] added to cache.")

        Some(result)
    }
  }
}

@Entity
@NamedQueries(
  Array[NamedQuery](
    new NamedQuery(name = "findMappedValueByType", query = "FROM MappedValue WHERE mapType = :mapType")
))
@Table(name = "MappedValue")
class MappedValue{
  @Id
  @GeneratedValue(strategy=GenerationType.IDENTITY)
  @Column(name = "mvd_id", updatable = false)
  @BeanProperty var id: Int = _

  @Column(name = "mvd_type")
  @BeanProperty var mapType: String = _

  @Column(name = "mvd_ref_id")
  @BeanProperty var refID: String = _

  @Column(name = "mvd_use")
  @BeanProperty var use: String = _

  @Column(name = "mvd_label")
  @BeanProperty var label: String = _
}

And the test class:

@RunWith(classOf[JUnitRunner])
class ValueMapperTest extends FunSuite with DBTest with BeforeAndAfter with Matchers {

  var logger: Logger = null
  var testService: ValueMapper = null

  before {
    ModuleLoader.shutdown()
    Thread.sleep(500)

    makeDBCopy("testPersStoreMaster1", "vmtPersStore")

    val resourceURL = getClass.getResource("ValueMapperTestManifest.xml")
    ModuleLoader.initialize(resourceURL)

    val defaultContext = ModuleLoader.getDefaultContext
    val loggerFactory = defaultContext.findService(classOf[LoggerFactoryService]).get
    logger = loggerFactory.getLogger(getClass.getName)

    defaultContext.findService(classOf[ValueMapper]) match {
      case None => fail("Should have found service to test.")
      case Some(s) => testService = s
    }
  }

  val fromData = List("Raid", "Appliance", "IsilonProprietaryHST", "IsilonPublicHST", "IsilonPublicFUSE", "IsilonRefFilesHST", "SafeStoreCache")
  val fullToData = List("Archive (Primary)", "Safe Store", "Isilon (HST Proprietary)", "Isilon (HST Public)", "Isilon (FUSE Public)", "Isilon (HST Ref Files)", "(obsolete) safe store cache")
  val filteredToData = List("Archive (Primary)", "Safe Store", "Isilon (HST Proprietary)", "Isilon (HST Public)", "Isilon (FUSE Public)", "Isilon (HST Ref Files)")

  test("mapping"){
    var result = testService.map("StorageType", fromData, false)
    result should be(fullToData)

    result = testService.map("StorageType", fromData, true)
    result should be(filteredToData)

    an [IllegalArgumentException] should be thrownBy {testService.map("BadType", fromData, true)}
  }

  test("individual map") {
    testService.get("StorageType", "Raid", false).get should be ("Archive (Primary)")

    testService.get("StorageType", "SafeStoreCache") should be (None)

    testService.get("BadType", "Something") should be (None)
  }
}

Answer 1

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

Answer 2

    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 <- cacheGet(typeID);
      record <- data.get(key)
    ) {
      if (record.getUse != "n" || !ignoreUnused) result = Some(record.getLabel)
    }

    result
  }

Can be transformed to:

// above in code
type ValueMapperType = String
// ...


    def get(typeID: ValueMapperType, valueMapperKey: String, ignoreUnused: Boolean = true): Option[String] = {
    logger.trace("[get] looking for: {} in {}", valueMapperKey, typeID, "")
    for {
      cache <- cachedMappersByType(typeID)
      record <- cache.get(valueMapperKey) if record.getUse != "n" || !ignoreUnused
    } yield record.getLable
}