1
\$\begingroup\$

I've to create a class MyCalculator which consists of a single method long power(int, int). This method takes two integers, and, as parameters and finds Math.pow(n,p). - If either n or p is negative, then the method must throw an exception which says "n or p should not be negative.". - If both n and p are zero, then the method must throw an exception which says "n and p should not be zero."

class MyCalculator {
    /*
    * Create the method long power(int, int) here.
    */
    long power(int n, int p) throws Exception {
        if(n==0 && p==0) {
            throw new Exception("n and p should not be zero.");
        } else if(n<0 || p<0) {
            throw new Exception("n or p should not be negative.");
        } else {
            return (long)Math.pow(n, p);
        }
    }
}

Is there any way to use Exception handling in a better way in such questions? We have to perform this check in most of the questions. So I need to understand it clearly.

\$\endgroup\$
  • \$\begingroup\$ How does adding a Custom Exception in such cases sound? I know that's a bit extra work, but when is it really worth adding a Custom Exception? \$\endgroup\$ – user2769790 Oct 10 '17 at 14:59
  • \$\begingroup\$ I don't understand why those exceptions are necessary, and how they improve Math.pow(). Why should it be an error to raise a negative integer to a positive power? \$\endgroup\$ – 200_success Oct 12 '17 at 3:12
  • \$\begingroup\$ I totally agree with you. Actually, the focus here is how to better use Exception Handling in such case scenarios. \$\endgroup\$ – user2769790 Oct 12 '17 at 14:09
  • \$\begingroup\$ What do you mean by "such case scenarios"? My point is that this scenario doesn't even make sense. \$\endgroup\$ – 200_success Oct 12 '17 at 14:10
  • \$\begingroup\$ I can go ahead and remove that negative check condition from the question but maybe that was how it was asked to handle, by not allowing any of the inputs to be negative. As I said, the question is not how to improve Math.pow(), there could be anything else in place of Math.pow(), but just to make use of Exception Handling more, this negative condition was taken. \$\endgroup\$ – user2769790 Oct 12 '17 at 14:24
4
\$\begingroup\$

Is there any way to use Exception handling in a better way in such questions?

What you have is a very good start.

I would suggest refining your exception type where possible. In your case, I would suggest using IllegalArgumentException. Using a specific subtype helps with diagnosing/debugging.

Additionally, since IllegalArgumentException is a type of RuntimeException, you can then remove the throws Exception from your method signature.

long power(int n, int p) {
    if (n == 0 && p == 0) {
        throw new IllegalArgumentException("n and p should not be zero.");
    } else if (n < 0 || p < 0) {
        throw new IllegalArgumentException("n or p should not be negative.");
    }

    return (long) Math.pow(n, p);
}
\$\endgroup\$
0
\$\begingroup\$

Just a little improvement of in the code extending from JvR's answer: I think it's possible to get rid of the else if since you're throwing exceptions for invalid inputs. I do agree with JvR in using a specific subtypes for exceptions. But I think it's best if you create your own exception extending from IllegalArgumentException like so:

public class InvalidCalculatorInputException extends IllegalArgumentException{
    public InvalidCalculatorInputException(String message){
        super(message);         
    }
}

Also, I think it's best if you separate the logic for handling the exceptions from the power method. This is to make the power method more readable. Applying all the things I've mentioned, this is my refactored version of your MyCalculator class:

class MyCalculator {
    long power(int base, int exponent) {
        checkIfInputsAreValid(base, exponent);
        return (long) Math.pow(base, exponent);
    }

    private void checkIfInputsAreValid(int base, int exponent){
        if(base ==  0 && exponent == 0)
            throw new InvalidCalculatorInputException("The base and the exponent should not be both 0.");
        if(base < 0 || exponent < 0)
            throw new InvalidCalculatorInputException("Either base or exponent should not be negative. Base is " + base + "; exponent is " + exponent);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I didn't find the class InvalidArgumentException in the Java API. Did you mean IllegalArgumentException? \$\endgroup\$ – Ralf Kleberhoff Oct 12 '17 at 19:03
  • \$\begingroup\$ oh yeah, it's IllegalArgumentException lol. My bad :)) \$\endgroup\$ – Rocket Pingu Oct 12 '17 at 21:30
0
\$\begingroup\$

You might consider using custom exceptions coupled with an assertion class. This would give you a lot of good exception type information to catch and work with, instead of just descriptions in a single exception type. As well, it cleans up your method nicely by stating in English what is happening.

public class MyCalculator
{   
    public long Power(int n, int p) 
    {
        Assert.AreNotZero("n and p", n, p);
        Assert.AreNotNegative("n and p", n, p);

        return (long)Math.Pow(n, p);        
    }   
}

public static class Assert
{
    public static void AreNotZero(string arg, params int[] values)
    {
        if (values.Where(v => v == 0).Any())
            throw new InvalidZeroException(arg);
    }

    public static void AreNotNegative(string arg, params int[] values)
    {
        if (values.Where(v => v < 0).Any())
            throw new InvalidNegativeException(arg);
    }
}

public class InvalidZeroException : Exception
{
    public InvalidZeroException(string argument) : base(argument + " should not be zero") { }
}

public class InvalidNegativeException : Exception
{
    public InvalidNegativeException(string argument) : base(argument + " should not be negative") { }
}
\$\endgroup\$
  • \$\begingroup\$ This is about Java, not C#. :) \$\endgroup\$ – Roland Illig Oct 13 '17 at 6:55
  • \$\begingroup\$ @RolandIllig i believe it's about good exception practices and the information is translatable. \$\endgroup\$ – TBone Oct 13 '17 at 11:28
0
\$\begingroup\$

If the error handling takes too much space in your code, you can invent a little helper function.

private static void ensure(boolean condition, String message) {
    if (!condition) {
        throw new IllegalArgumentException(message);
    }
}

Using this helper method, you can write the power function like this:

long power(int n, int p) {
    ensure(n != 0 || p != 0, "n and p should not be zero.");
    ensure(n >= 0, "n should not be negative.");
    ensure(p >= 0, "p should not be negative.");
    return (long)Math.pow(n, p);
}

Note that the messages are all string literals. If you were to use a string expression like "n is negative: " + n instead, this string would be computed on every invocation of power, which would make your code very slow. Using string literals, on the other hand, doesn't cost any time.

\$\endgroup\$
0
\$\begingroup\$

Thanks for sharing your code.

for your request of reusing the checks you have 2 options.

Either one requires that you move the checks into a parameterized method first.

Before you can do that you have to understand that the current method is exited immediately when an exception is thrown. This means than you don't need the final else :

class MyCalculator {
    long power(int n, int p) throws Exception {
        if(n==0 && p==0) {
            throw new Exception("n and p should not be zero.");
        } else if(n<0 || p<0) {
            throw new Exception("n or p should not be negative.");
        } 
        return (long)Math.pow(n, p);
    }
}

Now you can select the complete if/else and invoke your IDE's extract method refactoring (eg. in eclipse press <1> to bring up the quickfix menu and find the refactoring there).

This will change your code to this:

class MyCalculator {
    long power(int n, int p) throws Exception {
        verifyOperandsPreconditions(n, p);
        return (long)Math.pow(n, p);
    }
    private void verifyOperandsPreconditions(int n, int p) throws Exception {
        if(n==0 && p==0) {
            throw new Exception("n and p should not be zero.");
        } else if(n<0 || p<0) {
            throw new Exception("n or p should not be negative.");
        } 
    }
}

From now on you have 2 was to go:

  1. legacy inheritance approach
    In this solution you create a super class with the check method and a subclass that does the actual work:

    This will change your code to this:

    class MyCalculatorBaseClass {
        // mind the new scope!
        protected void verifyOperandsPreconditions(int n, int p) throws Exception {
            if(n==0 && p==0) {
                throw new Exception("n and p should not be zero.");
            } else if(n<0 || p<0) {
                throw new Exception("n or p should not be negative.");
            } 
        }
    }
    
    class MyCalculator extends MyCalculatorBaseClass {       
        long power(int n, int p) throws Exception {
            verifyOperandsPreconditions(n, p); // call to base class method
            return (long)Math.pow(n, p);
        }
    }
    
  2. modern composition approach
    In this solution you use MyCalculatorBaseClass as a dependency in your MyCalculator class:

     class MyCalculatorChecks {
        // again we change the scope!
        public void verifyOperandsPreconditions(int n, int p) throws Exception {
            if(n==0 && p==0) {
                throw new Exception("n and p should not be zero.");
            } else if(n<0 || p<0) {
                throw new Exception("n or p should not be negative.");
            } 
        }
    }      
    
    class MyCalculator {   
        MyCalculatorChecks checks = new MyCalculatorChecks();    
        long power(int n, int p) throws Exception {
            checks.verifyOperandsPreconditions(n, p); // call to method on dependency
            return (long)Math.pow(n, p);
        }
    }
    

About the visibility scopes:

In both example we had to change the visibility scope of the extracted method in order to enable access from other classes.

I choose protected in the first approach to signal that this method is meant to be used by sub classes.

I choose public in the second approach to signal that this method is meant to be used by any other class.


Which of the two approaches you mentioned is better? "modern composition approach or legacy inheritance approach" – user2769790

The "modern composition approach" of cause. Thought my wording was clear enough...

And do we really need to create a separate class for a single method? What's a good practice for interview whiteboard preparation? – user2769790

If there is only one method to be shared: Yes.

But most likely you will collect several methods in one object/class.

On the other hand the Single Responsibility Pattern must be applied to that common class too which means that you might sometimes need more than one class for such common behavior.

\$\endgroup\$
  • \$\begingroup\$ > Which of the two approaches you mentioned is better? "modern composition approach or legacy inheritance approach" > And do we really need to create a separate class for a single method? What's a good practice for interview whiteboard preparation? \$\endgroup\$ – user2769790 Oct 11 '17 at 21:31
  • \$\begingroup\$ (While there is "to exact", that probably should read extracted: never trust a spelling checker to catch all typing errors. And of cause still is wrong, of course, as is "the second" tho.) \$\endgroup\$ – greybeard Oct 12 '17 at 10:56
  • 1
    \$\begingroup\$ I downvoted your answer because I think it violates readability, which to me is the most important feature of professional code. In both variants you have to check two classes until you see that the calculator throws an exception in specific cases. \$\endgroup\$ – Ralf Kleberhoff Oct 12 '17 at 18:55
  • \$\begingroup\$ @RalfKleberhoff could you please explain how this affects readability in a bad way? \$\endgroup\$ – Timothy Truckle Oct 12 '17 at 19:28
  • \$\begingroup\$ Having a method called extracted at the end of your answer is unacceptable. It's only acceptable for a tiny moment for explaining how to fix up the method name after doing a refactoring like this. \$\endgroup\$ – Roland Illig Oct 12 '17 at 21:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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