0

There's a function in our codebase with a signature like this:

def hasPermission(...): Try[Unit]

It basically checks if a user has permission to perform a certain action on a certain item. If the user has permission, it returns an empty Success, and if not, it returns a Failure with a specific exception type. This function is often used within comprehensions like this:

for {
    _ <- hasPermission(...)
    res <- doSomething()
} yield res

This seems like bad practice, but I can't quite articulate why I feel that way. To me, it seems like hasPermission should simply return a Boolean.

Is this an appropriate use of a Try?

edit: I think my question is different than the linked one because it's more specific. That one is asking a general question about returning Try[Unit], which I believe is acceptable in some cases.

mplis
  • 748
  • 2
  • 6
  • 19
  • A boolean could only convey that a failure occurred, not the cause of the failure. You could throw an exception describing the failure, but whether that's good practice is up for debate... The fact that no useful result (`()`) is returned in case of success is largely irrelevant. – jub0bs May 02 '17 at 14:57
  • 1
    Possible duplicate of [Is using Try\[Unit\] the proper way?](http://stackoverflow.com/questions/23898507/is-using-tryunit-the-proper-way) – jub0bs May 02 '17 at 14:59
  • @mplis In full honesty no, not at all. Flow control done properly is usually a combination of monads/monad transformers. It's always hard to express the proper amount of information to represent error states: https://www.47deg.com/blog/fp-for-the-average-joe-part-2-scalaz-monad-transformers/ Sometimes you want applicatives, sometimes you want to short circuit etc, there's no generic answer, but if I saw `Unit` return type in the codebase, your code would get replaced 3 seconds later. that's a very binary approach, but that's the way it is. – flavian May 02 '17 at 15:11

2 Answers2

2

If the method says hasPermission, then I'd say it should return a Boolean, or a Try[Boolean]. Try[Unit] is not as obvious as Try[Boolean], and the caller would have to inspect the exception to tell if it didn't have the permission, or whether it failed to retrieve the permission info.

Now that said, generally calling hasPermission and then acting depending on the result can cause race conditions (e.g. if the permission is revoked after hasPermission is called). Therefore it's often preferable to do def doSomething(...): Try[Unit] and then raise e.g. a NoPermissionException.

Enno Shioji
  • 26,542
  • 13
  • 70
  • 109
0

Generally, the use of exceptions for control flow is an anti-pattern

Try tries (no pun intended) to encapsulate that flow control, but if you don't actually need to use exceptions, there's no reason to. Scala 2.12's Either implementation seems close to what you probably want:

Either is right-biased, which means that Right is assumed to be the default case to operate on. If it is Left, operations like map and flatMap return the Left value unchanged:

Let's assume that you are implementing a web server, and this logic is in control of a particular path:

type Request = ...
type Response = String
type ResponseOr[+T] = Either[Response, T]

def checkPermission: ResponseOr[Unit] = 
  if(hasPermission) Right(())
  else Left("insufficient permissions")

def doSomething(req: Request): ResponseOr[Something] = 
  if(argumentsAreBad(req)) Left("arguments are bad!")
  else Right(new Something)

def makeResponse(result: Something): Response = ???

def handleIt(req: Request): Response = {
  val result = for {
    _ <- checkPermission
    result <- doSomething
  } yield makeResponse(result)
  result.merge // special method for `Either[T, T]` that gives a `T`
}

You'll see similar behavior to Success vs Failure - think of Left as analagous to a Failure, where if at any point one of the flatMap/map steps returns a Left, that's the end result and the rest are "skipped". No exceptions required.

Dylan
  • 13,645
  • 3
  • 40
  • 67