In an effort to improve my programming I am trying out this problem in Java. I have looked everywhere for an elegant solution and cant find it.
Write a program which takes a single argument, computes the factorial and prints the value on the screen.Also the program must be interactive instead of terminating after each computation and that it uses a table of previous values to improve the performance.
How do I make it so that in the method calls it checks if it has already computed the factorial for a value? Any other improvements to this code to make it more readable or efficient are also appreciated.
This is what I have written so far.
public static void main(String[] args) {
int input = 0;
Scanner reader = new Scanner(System.in);
HashMap cache = new HashMap();
while(1 == 1){
do{
try {
System.out.println("Enter the number you would like to factor");
input = reader.nextInt();
} catch (InputMismatchException e){
reader.next();
}
} while(input <= 0);
if(cache.get(input) != null){
System.out.println("Found in cache");
System.out.println(input + "! = " + cache.get(input));
}
else {
cache.put(input, factorialRecursively(input));
System.out.println(input + "! = " + factorialRecursively(input));
}
}
}
public static int factorialIteratively(int a) throws InputMismatchException {
int temp = 1;
while(a > 0){
temp = temp * a;
a--;
}
return temp;
}
public static int factorialRecursively(int a) throws InputMismatchException{
if (a == 1){
return 1;
}
else {
return a * factorialRecursively(a-1);
}
}
OK, I'll give it a shot. Let me first give you the full code answer, and get into each change in details:
public static void main(String[] args) {
int input = 0;
try (Scanner reader = new Scanner(System.in)) {
Map<Integer, Integer> cache = new HashMap<Integer, Integer>();
cache.put(1, 1);
while (true) {
do {
try {
System.out.println("Enter the number you would like to factor");
input = reader.nextInt();
} catch (InputMismatchException e){
reader.next();
input = 0;
}
} while(input <= 0);
if(cache.get(input) != null) {
System.out.println("Found in cache");
System.out.println(input + "! = " + cache.get(input));
}
else {
int result = factorialRecursively(input, cache);
System.out.println(input + "! = " + result);
}
}
}
}
public static int factorialRecursively(int limit, Map<Integer, Integer> cache) {
int cacheSize = cache.size();
if (cacheSize < limit) {
int nextFactorial = cacheSize + 1;
System.out.println("Calculating and caching " + nextFactorial + "!");
cache.put(nextFactorial, cache.get(cacheSize) * nextFactorial);
return factorialRecursively(limit, cache);
}
else {
return cache.get(limit);
}
}
1) the direct answer to your question: "how to make sure the method that calculates the factorial, checks whether it has already computed the factorial for a value". So I stuck to the recursive approach, although you could also implement the iterative version using the same concept.
Basically, the idea is to pass a reference of the results cache to the method itself (the Map<Integer, Integer> cache argument), so it becomes responsible for storing the resulting computed factorial at each multiplication step, and not only once the full factorial has been computed. To do so, I made the method compute the factorial from the bottom up, and not the other way round. We store the smallest factorial in the cache as starting point (cache.put(1, 1);). I added an output message which shows when a factorial is calculated and cached, and by testing the code you can see that one particular factorial only ever gets calculated and stored once.
2) the use of Map<Integer, Integer> cache = new HashMap<Integer, Integer>(); instead of HashMap cache = new HashMap();.
a) Since Java 1.5, collections have been made generic (long story, outside the scope here), and when referenced should be parameterized. So here we're saying, I want to store Integer values (computed factorials), that I can access with Integer keys (factorial source).
b) The reason the code compiles and allows us to use integer primitives and not Integer class objects, is because of a mechanism named auto-boxing which seamlessly wraps integer primitives into Integer objects, and vice-versa. Again, out of scope, but one should be aware of it.
c) The functionality we are after here is that of the Map interface, which HashMap happens to be an implementation of. Unless the referencing code relies on HashMap specific methods (unlikely), it is good practice to declare your variables as the corresponding collection interface (like List for ArrayList etc.), not as the implementation class.
3) InputMismatchException is only thrown by the Scanner class, and therefore does not need to be thrown by any code that does not call the Scanner nextInt() method.
4) the use of true instead of 1 == 1. Equivalent in practice, but using a boolean value is more readable.
5) Only calling the factorialRecursively method once. Your code calls the method twice, which is unnecessary. If you're after efficiency and elegance, you should only call the method once and store its results in a variable.
6) the use of try (Scanner reader = new Scanner(System.in)) { ... instead of Scanner reader = new Scanner(System.in);. A Scanner object must always have its close() method called to clean up resources. In older Java versions this was achieved using try{}finally{} statements, but later versions allow this syntax (try-with-resources statement) if the class implements the AutoCloseable interface. This means the close() method is guaranteed to always get called.
7) Last but not least: the use of int and Integer, which I did not change. As suggested in the comments by @indivisible, you should be aware of data type limitations. An int can only hold numbers represented by 31 bits + sign (-2^31 -1 to 2^31), which limits this code to 16!, after which results become erroneous. If you wish to generate bigger numbers you should use long/Long. Anyway, again out of scope of this question really.
Have fun :)