4

I'm working on a Scala project and Wartremover shows an error about Option#get usage in my code:

Option#get is disabled - use Option#fold instead

While I do understand how get should often be avoided, I think there are cases where it's reasonable to use it, like my current one: First, I create an answer item (in the database), then I want to return the freshly created answer by reading it from there.

def create(answer: Answer): Future[Answer] = {
  writeToDb(answer) // returns Future[Long]
    .flatMap(
      readFromDb(_) // returns Future[Option[Answer]]
        .map(_.get) // Wartremover complains here
    )
}

My understanding is that get should generally be avoided because it breaks control flow in case of None, as an exception is thrown. However, I expect my Option here to always contain an answer, as I just created it. If it's not there, there's likely a bug in my code or an issue with the database. In such a case it would be unreasonable to fall back to a default value or null. I'd much rather throw an exception or directly map to a failing future.

An alternative to get is explicitly matching agains the option:

def create(answer: Answer): Future[Answer] = {
  writeToDb(answer) // returns Future[Long]
    .flatMap(
      readFromDb(_) // returns Future[Option[Answer]]
        .flatMap {
          case Some(answer) => Future.successful(answer)
          case None => Future.failed(
            new IllegalStateException("Failed to load answer after creation"))
        }
    )
}

But this is way more verbose while achieving almost the same as a simple _.get.

Am I missing something here? Or is this just a false positive from Wartremover?

  • 1
    This sounds to me like a question that would be better asked on Stack Overflow. However, I know next to nothing about Scala, so ignore me if this is not the case. – David Arno Feb 1 '18 at 9:01
  • Why do you even need to re-read from the database? Getting a generated ID? – Sebastian Redl Feb 1 '18 at 9:14
  • 1
    @DavidArno It's about best practices with regard to functional programming, so I thought it fits best here. – Cedric Reichenbach Feb 1 '18 at 9:18
  • 1
    @CedricReichenbach But unless this whole function is in an isolated transaction, your query could, in fact, return null. There's a race condition with another process deleting the entry immediately after it was added. – Sebastian Redl Feb 1 '18 at 9:23
  • 2
    You get the benefit of a specific exception with the initial answer as context, rather than a general exception of "get on an empty Option[Answer]" – Caleth Feb 1 '18 at 9:44
6

Instead of pattern matching explicitly, you can write

.getOrElse { throw new IllegalStateException("Failed to load answer after creation") }

More generally, when dealing with Option/Either/Try you can often avoid pattern matching by looking for an existing method and reserve it for cases where you need to do something non-trivial.

I wouldn't really consider it a false positive: being explicit about the exception is good and will make it easier to find and add more debugging information if you do run into a problem.

  • Ok, I see the point about explicit exceptions. However, this will now get rejected by Wartremover because of the Throw rule, which forbids any throwing. – Cedric Reichenbach Feb 1 '18 at 13:22
  • @CedricReichenbach Wartremover rejects any throwing, or just throwing inside an Option? – Andres F. Feb 1 '18 at 14:59
  • @CedricReichenbach The motivation for this rule is "throw implies partiality. Encode exceptions/errors as return values instead using Either." and a failed future works fine for this. So it doesn't particularly apply. – Alexey Romanov Feb 1 '18 at 16:11
  • 1
    @CedricReichenbach I personally wouldn't enable Throw rule in the first place, but your mileage may vary (or you may be working on a project which already came to decision to enable it). – Alexey Romanov Feb 1 '18 at 16:26
  • @AlexeyRomanov Thanks for the input; I just integrated Wartremover and defaulted to Wart.unsafe (all rules considered stable), but am now finding several ones I find not reasonable and will probably disable. – Cedric Reichenbach Feb 1 '18 at 16:29

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.