current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am using play framework with scala and here is a scenario on which I would like to have your opinion.

Let's say I have 2 custom exceptions that I can throw, which are EmailTaken and AuthFail. What I need to do here is, whenever any of these 2 exceptions happen, I need to return a json with 3 keys success, errorCode and message, in case of all other exceptions let Play's default error handling take charge. Now for all custom exceptions success will always be false, each exception would have a unique errorCode integer value and a unique message string both of which remain fixed for a particular exception.

Now I saw people using case class(they can be pattern matched) or class(saw some java examples) for exception but I didn't see the point of creating them because for any particular exception I don't see the point of creating more than 1 object because its attributes don't have to be changed (I could create a case class with all default parameters but why?). So I created an object for each exception and then I throw that exception object in the controller and then in the Global object I rely on the getCause of the exception.

// custom exceptions code

trait AppException extends Exception {

  val success = false
  def errorCode: Int
  def message: String

  override def fillInStackTrace(): Throwable = {
    this
  }
}

object EmailTaken extends AppException {
  val errorCode = 1
  val message = "email already used"
}


object AuthFail extends AppException {
  val errorCode = 2
  val message = "id/password dont match"
}

// in Play's controller

object Application extends Controller {
def test = Action {
    throw EmailTaken
    Ok("Test Page")
  }
}

// in Play's Global object

object Global extends GlobalSettings {

  override def onError(request: RequestHeader, ex: Throwable) = {
    ex.getCause match {                
      case c: AppException =>     // a known custom exception
        Future.successful(InternalServerError(
          Json.toJson(Json.obj(
            "success"-> c.success,
            "message"-> c.message,
            "errorCode"-> c.errorCode
          ))
        ))
      case _ =>
        super.onError(request, ex)
    }
  }
}

I understand that if AuthFail was A case class I would have returned new AuthFail() and pattern matched on that exception object. Also, I could have used isInstanceOf[AppException] to check for custom exceptions. But are there any problems with this approach? Also, what are the other good approaches I can take? I don't have much experience with Scala or Java.

share|improve this question

1 Answer 1

up vote 1 down vote

You are right to not create classes for AuthFail and EmailTaken. These really sense as instances, rather than classes, as they are special cases of AppException.

For example if AppException was a class, then AuthFail and EmailTaken would be best as instances of that class:

class AppException(val errorCode: Int, val message: String) extends Exception {
  val success = false
}

val emailTaken = new AppException(1, "email already used")

val authFail = new AppException(2, "id/password dont match")

However, if they are instances, then you would need to pass them around in the code where they might be needed. Another alternative is to make them objects, so you can easily import them and use them where needed:

object EmailTaken extends AppException(1, "email already used")

object AuthFail extends AppException(2, "id/password dont match")

Your approach using a trait for AppException instead of a class seems fine too, but I'm fairly new to Scala myself. Using a class like I did above seems shorter, so I would prefer it.

As for using case classes (or case objects), in the posted code I don't see a need: it seems that you don't need to differentiate the different cases. If all of these cases can be handled as an AppException and result in an InternalServerError, then there's no need to implement more.

Finally:

I could have used isInstanceOf[AppException] to check for custom exceptions. But are there any problems with this approach?

Don't do that. It's recommended to avoid isInstanceOf as much as possible. The alternative is using polymorphism or pattern matching. In your case, you don't actually need any of this. You have simple instances of AppException: all your cases behave the same way, their only difference is in parameters, not in behavior.

share|improve this answer
    
One of my concern was using getCause of the exception. I haven't seen it being used anywhere till now. Can the pattern match on getCause be unreliable sometimes? –  lovesh Apr 25 at 8:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.