[SOLVED] Chained if-statements vs. for comprehension

Design
1

Let’s say I want to check, whether a path exists, is a directory, and can be written.

Until now I’d have solved this task this way:

def checkPath(path: Path): Either[ErrorMessage, Unit] =
  if (Files.exists(path) == false) {
    Left(ErrorMessage("does not exist"))
  } else if (Files.isDirectory(path) == false) {
    Left(ErrorMessage("not a directory")
  } else if (Files.isWritable(path) == false) {  
    Left(ErrorMessage("is not writable")
  } else {
    Right(())
  }

The same thing can be accomplished with a for comprehension:

def checkPath(path: Path): Either[ErrorMessage, Unit] =
  for {
    _ <- if (Files.exists(path) == false) Left(ErrorMessage(..)) else Right(())
    _ <- if (Files.isDirectory(path) == false) Left(ErrorMessage(..)) else Right(())
    _ <- if (Files.isWritable(path) == false) Left(ErrorMessage(..)) else Right(())
  } yield ()

Now my question is: Which of the two versions is the better one? By ‘better’ I mean: What feels more natural to experienced Scala developers?

(Does anyone know why the syntax highlighting has different results for both code snippets?)

2

First, I always find it distracting to check whether a boolean result “is equal to false” / “is equal to true”. It’s a boolean already and it reads more naturally to me using if (isWritable(x)) than if (isWritable(x) == true)).

Then, there is Either.cond which you could use instead, to reduce some verboseness:

def checkPath(path: Path): Either[ErrorMessage, Unit] =
  for {
    _ <- Either.cond(!Files.exists(path), ErrorMessage(..), ())
    _ <- Either.cond(!Files.isDirectory(path), ErrorMessage(..), ())
    _ <- Either.cond(!Files.isWritable(path), ErrorMessage(..), ())
  } yield ()

But, using Unit as the right type inside the Either feels weird.

And, checking such conditions before actually doing something with the file / path is just asking for trouble, since the world can change between your check, and the action being carried out. Just try to use the file / directory as you intend and check for exceptions (you have to do this anyways), depending on the type of exception, return an “does not exist” error or some other message.

3 Likes
3

I know you don’t want to pull in external libraries - still, just to mention the alternative…

def checkPath(path: Path): Either[ErrorMessage, Path] = {
  import cats.syntax.all._
  import Files._
  path
    .asRight[String]
    .filterOrElse(exists(_), "does not exist")
    .filterOrElse(isDirectory(_), "not a directory")
    .filterOrElse(isWritable, "is not writable")
    .leftMap(ErrorMessage)
}
2 Likes
4

Agreed, I’d return the “validated” path on the rhs. In the spirit of “Parse, don’t validate” I might even be tempted to wrap it in some ValidatedPath type, but…

Agreed, again. Checks like this may make some kind of sense in some cases, though, for failing fast and/or in order to provide more meaningful error messages.

5

You don’t necessarily need cats for this:

Right(path)
  .filterOrElse(exists(_), "does not exist")
  .filterOrElse(isDirectory(_), "not a directory")
  .filterOrElse(isWritable, "is not writable")
  .left.map(ErrorMessage)

Syntax highlighting in Discourse is crap for some reason.

Usually in markdown you can add scala after the first 3 backticks to select your language but Discourse either doesn’t know scala or just ignores the annotation. Or both.

2 Likes
6

It might surprise you, but just the other day I ventured into learning Cats Effects :wink:

7

I believe that’s a question of prefernces. I, on the other side, cannot stand the notation with the exclaimation mark and rather see a full comparison statement. But as I said, everyone is different.

I actually don’t see the point in returning anyhting else than Unit in this case, since nobody else is going to use the return value. But again, I have seen both and I for me the most important thing is to be consitend throughout a code base.

I did not know about the Either.cond, I would not have even thought about this. Thank you for pointing it out!

1 Like
8 
def writeData(data: String)(targetDir: Path): Either[ErrorMessage, Unit] = ???

checkPath(path).flatMap(writeData("foo"))
1 Like
9

In the end both achieve the final result, I personally would not use for in this case; since, in general, I expect a for to flow most of the time instead of failing fast most of the time, also the for version requires more boilerplate. But, really use whatever makes more sense to you and your team.

I disagree, this is a common linting rule in all languages I know.

1 Like