CODEWITHSUNDEEP
java
mohan
mohan

Reputation: 269

Is it good catch IllegalArgumentException

I have a method m1() which throws IllegalArgumentException. I am invoking this method from some other method called m2, which throws HttpException and Exception.My requirement is that i need to throw HTTPException whenever there is IllegalArgmumentException from m1() while invoking from m2().

The approach i added is as below.Placing the invocation inside try block and if there is any exception then immediately create new HTTP exception and throw it .

public static CustomizedEnum m1(String value) {
        if (value == null){
            throw new IllegalArgumentException(" Input can not be  not be null.");
        }

        if (value.equals("ABC")) return ABC;
        if (value.equals("XYZ")) return xyz;
        if (value.equals("PQR")) return pqr;

        // no matching string, throw an exception
        throw new IllegalArgumentException( value + 
                     " for m2() in CustomizedEnum does not exist."); 
    } 

void m2() throws HttpException,Exception {
   // ..............
   //.................
   //................
  try{
  if(m1("ABC") != null){
  }catch(Exception e){
     throw new HTTPException("Invalid data");
  } 
     // ............
     //................   
}

Would like to know is this above approach is best approach or not. If this approach is not good one please suggest some better approach.

Upvotes: 2

Views: 1375

Answers (3)

Dici
Dici

Reputation: 25980

Following Bill K's comment, I agree that in your case you are probably using exceptions as control-flow, which isn't a great idea:

  • exceptions are made for errors, not conditional branching. You most often don't want to recover from an exception (by recover I mean continue as if nothing happened), at best you want your application to survive i.e. not crash, that's pretty common if it's a service or an app.
  • exceptions are a lot slower than if/else branching
  • it's just verbose

Now I want to make a larger point about exception handling because it is possible that m1 throws an exception that you had not foreseen initially, and if m2 is exposed to external clients you probably don't want them to see random errors from your service. That's why I would add a "catch-all" and rethrow as a more general error such as InternalFailure.

Furthermore, don't forget to always include the root cause of an exception when you rethrow it, otherwise you'll have a hard time figuring out what happened exactly:

throw new HTTPException("Invalid data", e);

The last problem in your code is that you're catching all exceptions as if they were necessarily about "invalid data", even though it might not be the case. You will therefore generate a confusing exception.

If we compile all those observations, it would give us something like that:

validateInputs(inputs);

try {
    if(m1(inputs) != null) { ... }
} catch (Throwable t) {
   String msg = "Call failed for inputs ...");
   log.error(msg, t); // log for service owner to investigate
   // InternalFailure(msg, t) if you don't mind passing the exact exceptions to clients, but I'd say it's not necessary
   throw new InternalFailure(msg);
}

private static void validateInputs(Inputs inputs) {
    if (notValid(inputs)) throw new HTTPException("Invalid data: " + inputs); 
}

That way, you are not mixing completely unrelated things with a "bad data" message, and you're still offering a clean API with only a known set of expected exceptions that could result from calling this method.

Upvotes: 1

computercarguy
computercarguy

Reputation: 2463

A general rule of thumb is to only catch exceptional exceptions. Essentially this means to catch only the exceptions you can't expect, otherwise you need to prevent them with logic.

Also, you should catch specific exceptions, rather than a generic Exception. If you don't know what exception you're catching, it's probably a sign that you should try to figure out why an exception is happening in the first place and try to prevent it.

The idea behind catching an exception is to prevent the code from crashing and quitting execution, as well as trying to recover from the error, with logging the error so the developer can try to fix the error in later versions (if possible).

You shouldn't be throwing an error unless you expect to handle it somewhere else in the code. Otherwise you are causing a crash that should be otherwise addressed.

Some exceptions can't be avoided, though. This can be from an interrupted network connection, corrupt file stream, out of memory, and plenty of others reasons. You should still try to prevent them, but it's not always possible.

The following link describes the conditions for, why, and how to catch exceptions better than I can.

https://stackabuse.com/exception-handling-in-java-a-complete-guide-with-best-and-worst-practices/

Upvotes: 3

Bill K
Bill K

Reputation: 62789

It may be a strange differentiation, but it isn't bad as long as the string you are passing is coming from code and not from user input.

The general idea is that you shouldn't ever use an exception as part of your program flow--in other words, you should be pretty sure you'll never get that exception unless you write bad code.

So if it's user input or coming from external data then you should manually check your values before calling that function, but throwing the exception is not a bad safety check to tell you (as a programmer) that you've done something wrong in development. When used in this way, you should use unchecked exceptions because you don't really have to even catch them--in fact you probably shouldn't! Just let it crash your program with a stack trace so you can fix it.

Upvotes: 1

Related Questions