Simple factorial program using recursion

Question

Two concepts I realized I needed to understand and use more are recursion and Exceptions. Thus, I combined both in the following program. Although it began with a focus on using recursion it became more about Exceptions. I think the recursive part is bare-bones and straight-forward but I would nonetheless appreciate any tips/advice on using/formatting recursion in general.

On exception-handling: Is the following the optimal way of employing them or is there a more conventionally favored fashion? I get the feeling I may be missing doing something unnecessary or missing something important.

import javax.swing.JOptionPane;
class FactorialDemo {
 public static void main(String[] args) {
 int factorialNumber = 0;
 try {
 factorialNumber = Integer.parseInt(JOptionPane.showInputDialog(null,
 "Which number should we compute the factorial of?"));
 } catch(NumberFormatException nfe) {
 JOptionPane.showMessageDialog(null,"Input must be an integer!",
 "Error", JOptionPane.ERROR_MESSAGE);
 throw new NumberFormatException("Input must be an integer.");
 }
 JOptionPane.showMessageDialog(null,
 factorialNumber + " factorial is " +recur(factorialNumber) +".",
 "Result", JOptionPane.PLAIN_MESSAGE);
 }
 public static int recur(int n) {
 int result;
 if (n < 0) {
 JOptionPane.showMessageDialog(null,"n must be >= 0 but was " + n,
 "Error", JOptionPane.ERROR_MESSAGE);
 throw new IllegalArgumentException("n must be >= 0 but was " + n);
 }
 if (n < 2) {
 return 1;
 }
 result = recur(n - 1) * n;
 return result;
 }
}

Answer 1

Recursion

It's good that you are trying to grasp recursion, the more programming patterns you understand, the better.

But I would disagree that you need to use recursion more often. Generally, it consumes more memory and iterative solutions tend to be faster as well.

(削除) As for your solution, I think that if (n == 1) would be clearer than if (n < 2) (also, n < 2 returns 1 for input 0, which is wrong) (削除ここまで), and I wouldn't save the return value in result, but return it directly.

Exceptions

In recur throwing an IllegalArgumentException makes sense (it is an illegal argument), but showing the dialog here doesn't make that much sense, I would move that to main.

Also, don't throw an exception you just caught without a reason. If you don't want to execute the following function, just put all code in the try and catch the different exceptions.

Other

• recur isn't a good name. I would call it factorial or factorialRecursive.
• I would wrap the show message code into a function, that way you could also easily exchange it for something else (like a print). If you then further wrap this up in an object, unit testing would also be easier.

Code

If you do all of this, the code would look something like this:

public static void main(String[] args) {
 int factorialNumber = 0;
 try {
 factorialNumber = Integer.parseInt(input("Which number should we compute the factorial of?"));
 print(factorialNumber + " factorial is " + factorialRecursive(factorialNumber) + ".",
 "Result");
 } catch (NumberFormatException nfe) {
 error("Input must be an integer!", "Error");
 } catch (IllegalArgumentException iae) {
 error(iae.getMessage(), "Error");
 }
}
public static int factorialRecursive(int n) {
 if (n < 0) {
 throw new IllegalArgumentException("n must be >= 0 but was " + n);
 }
 if (n < 2) {
 return 1;
 }
 return factorialRecursive(n - 1) * n;
}
public static String input(String message) {
 return JOptionPane.showInputDialog(null, message);
}
public static void print(String message, String title) {
 JOptionPane.showMessageDialog(null,
 message,
 title, JOptionPane.PLAIN_MESSAGE);
}
public static void error(String message, String title) {
 JOptionPane.showMessageDialog(null, message,
 title, JOptionPane.ERROR_MESSAGE);
}

• Joffrey: IMO, IllegalArgumentException shouldn't be used this way. This is an exception to show the programmer that he did something wrong. The right way of doing it (IMO) is to test the user input before calling the method.
• tim: @Joffrey You wouldn't check the input in factorialRecursive at all (and then maybe throw a custom exception which extends Exception instead of IllegalArgumentException)? I always thought that relying on the calling code to get it right was not the correct approach? Also, my approach is in line with how parseInt handles it: If the input is not correct, it throws an unchecked exception. I could imagine adding additional code after parsing the input which checks if it positive, and that seems like a good approach, but also a bit like duplicating functionality.
• maaartinus: IMHO your error method should take the exception and optionally some details as arguments. Passing "Error" is redundant.
• tim: @maaartinus It's just a quick example (in real code, I would probably wrap it in a class and structure it quite a bit differently, but here I thought that it would be overkill). But I did think about your solution, and it would simplify the code. But the current error method is more flexible. It can have Important Error or System Error, or Exiting as title instead of an hardcoded Error title, and it can also print errors that do not come from exceptions (as it's doing right now in the case of NumberFormatException).
• Joffrey: @tim Exactly. In bigger applications, you would write methods like factorialRecursive with an IAE security, no matter where it gets its input from. On the other hand, each time you have user input, you test for correctness and send the right error messages before making methods crash with IAE. IAE would be here for you during the tests to ensure you didn't forget to test some user input, or to detect wrong calculations of the input if it is done in your program.

Answer 2

@tim's confusion in his answer indicates that your code could use a comment:

public static int recur(int n) {
 int result;
 if (n < 0) {
 JOptionPane.showMessageDialog(null,"n must be >= 0 but was " + n,
 "Error", JOptionPane.ERROR_MESSAGE);
 throw new IllegalArgumentException("n must be >= 0 but was " + n);
 }
 if (n < 2) {//both 0! and 1! are 1
 return 1;
 }
 result = recur(n - 1) * n;
 return result;
}

I was quite confused myself too.