Returning unnecessary empty stings in Match Case Scala-CodePudding

I have written some code that does not seem to be good practice. The code works, but is kind of a mess. I'm not used to the Some() and Option(). How can I reduce the amount of empty strings returned?

Here is the code

private def creatEvent(
          req: Request,
          tstamp: Date,
          ID: String,
          DB: Item,
          Drop: Item
  ): Unit = {
    val itemExpired: String = {
      Option(DB) match {
        case Some(item) =>
          Option(item.expires) match {
            case Some(expires) =>
              if (service.hasExpired(expires))
                " item expired."
              else
                ""
            case None =>
              ""
          }
        case None =>
          ""
      }
    }

CodePudding user response：

Assuming that there is an actual need to wrap DB and item.expires into Options (even though I have feeling that there isn't), here is a variant without redundant ""s:

val itemExpired: String = {
  val hasExpired = Option(DB).flatMap(item => Option(item.expires))
    .exists(service.hasExpired)
  if (hasExpired) " item expired." else ""
}

CodePudding user response：

Why do you need Option here at all? Do you expect that things you are wrapping into it could be null?

The best solution is to just never send nulls around in scala at all. Then you could just write:

if (service.hasExpired(DB.expires)) "item expired" else ""

If it is a possibility that either DB or DB.expires could be null, and you cannot fix that code, then flatMap is your friend:

def exp(it: Item) = Option(it).map(_.expires).flatMap(Option.apply)
if (exp(DB).exists(service.hasExpired)) "item expired" else ""

CodePudding user response：

I understand that you're not used to FP and Scala, but still among your question and all the answers, I can't understand why you should use empty strings? it's actually useless, it has no value. This kind of default values and using if/else expressions to determine weather the value is meaningful or meaningless is actually considered to be a bad practice in Scala. I would highly recommend you to use Options instead of empty strings:

val itemExpiredEvent: Option[String] = {
  Option(DB)
    .flatMap(item => Option(item.expires))
    .map(service.hasExpired)
    .flatMap {
      case true => Some("item expired")
      case _ => None
    }
}

So imagine this way, if you get None out of this, it's the empty string scenarios, otherwise, you have your value.

Let's compare the two approaches:

// if an events's value is not empty, we would send it to some server
// first approach
val event: String = getEvent(...)
if (event != "") {
  someClient.sendEvent(event)
} else {
  fallback // else logic, default or fallback value or something here
}

// this approach
val eventOpt: Option[String] = getEvent(...)
eventOpt
  .map(someClient.sendEvent)
  .getOrElse(fallback)

// if you do not want to cover else branch:
val event: String = getEvent(...)
if (event.nonEmpty) {
  someClient.sendEvent(event)
} // if blocks without else are very discouraged in Scala

// this approach
val eventOpt: Option[String] = getEvent(...)
eventOpt.map(someClient.sendEvent)
// or if sendEvent method returns Unit
eventOpt.foreach(someClient.sendEvent)