bad try catch block design?

Question

Whenever I use try-catch blocks in my code, my code always follows a pattern. First a try-catch block to open a resource followed by a null check and finally a try-catch block to read the resources. Is this a messy pattern? If so, what should the good design look like?

Here is an example of what I mean

public static void main(String[] args) {
    Process process = null;
    try {
        process = Runtime.getRuntime().exec("C:\\program.exe");
    } catch (IOException e) {
        e.printStackTrace();
    }
    if (process == null) {
        return;
    }
    try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
        String line;
        while ((line = reader.readLine()) != null) {
            System.out.println(line);
        }
    } catch (IOException e) {
        e.printStackTrace();
    }
}

@BevynQ that's an entirely different discussion http://stackoverflow.com/questions/4838828/why-should-a-function-have-only-one-exit-point — Koos Gadellaa, Jul 20 '16 at 06:35
If you get an exception while creating the process you should return from catch block, why to check for null on 'process' afterward? — A.B., Jul 20 '16 at 06:35

dimo414 · Accepted Answer · 2016-08-11T03:20:51.493

Your problem is that you are suppressing the exceptions in your code, which is why you need the null check. Particularly since this is in a main() method you could just let the exceptions propagate:

public static void main(String[] args) throws IOException {
  Process process = Runtime.getRuntime().exec("C:\\program.exe");
  try (BufferedReader reader =
      new BufferedReader(new InputStreamReader(process.getInputStream()))) {
    String line;
    while ((line = reader.readLine()) != null) {
      System.out.println(line);
    }
  }
}

This will still print a stack trace when there's an issue (since Java prints the stack trace of any exceptions that get thrown from main()), but you know that process can never be null. In general you should avoid over-eagerly catching exceptions - only catch them if there's something you can do about it. If not, pass them off to the caller (possibly wrapping them in a new exception type).

If you just don't care about the exceptions and don't expect them in practice you can make them RuntimeExceptions instead. This still lets you avoid needless null-checks but also doesn't require you to annotate your methods with throws:

public static void main(String[] args) {
  try {
    Process process = Runtime.getRuntime().exec("C:\\program.exe");
    try (BufferedReader reader =
        new BufferedReader(new InputStreamReader(process.getInputStream()))) {
      String line;
      while ((line = reader.readLine()) != null) {
        System.out.println(line);
      }
    }
  } catch (IOException e) {
    throw new IllegalStateException(
        "Unexpected exception while executing a subprocess", e);
  }
}

Oh I thought it was a good habit to catch all the exceptions and log them or something. — user2997204, Jul 20 '16 at 06:41
Sure, logging exceptions is a reasonable thing to do *if that's what you want to happen*. It's certainly not what you should do by default. Fail-fast by default, and suppress/log exceptions only if that's what you intend. — dimo414, Jul 20 '16 at 06:43

bad try catch block design?

1 Answers1

Linked