0

I'm fairly new to Java, and I'm trying to get my head around exceptions and when they should be used. I have been using them as a form of error checking, but I've come across several people saying that exceptions should only be used for things out of the program's control, such as user error.

I have a function which, for a given line in 2 dimensions, calculates all the y values between xMin and xMax. This function throws an exception if the line is vertical, as it is impossible to calculate all values of y on a vertical line. There is an equivalent function finding points between two y values as well, which throws an error if the line is horizontal.

findPointsInRangeOfX (int xMin, int xMax) throws LineIsVerticalException {
    // check if line is vertical
    // if vertical, throw an exception
    // else calculate the y values for each integer between xMin and xMax
    // return y values
}

I call this function as part of finding all the points on a line within a given window, given by a minimum and maximum x and y value. I don't check whether the line is vertical in this function; instead, I rely on the checking in findPointsInRangeOfX and use a try and catch block around the method.

pointsInWindow (int xMin, int xMax, int yMin, int yMax) {
    try {
        // Find list of all the points between xMin and xMax.
        // Remove from list all points not between yMin and yMax
    }
    catch (LineIsVerticalException e) {
        // If line is vertical, cannot find points between xMin and xMax
        try {
            // Instead, find list of all points between yMin and yMax
            // Remove from list all points not between xMin and xMax
        }
        catch (LineIsHorizontalException e) {
            // This part will never be reached because the line is vertical
            // But the compiler complains if the exception isn't caught
        }
    }
}

Is this ok? I'm not throwing the exception for an error as such - there's nothing wrong with having a vertical line - but I'm using it to tell pointsInWindow that it needs to find the points between the y values rather than the x values. Should I be duplicating the check to see if the line is vertical in the pointsInWindow function rather than using a try catch block? If I did duplicate the check, should I get rid of the LineIsVerticalException all together?

Emma
  • 335
  • 1
  • 4
  • 13
  • 2
    You might find some of this stack flow on when to throw exceptions interesting http://stackoverflow.com/questions/77127/when-to-throw-an-exception as well as this one with links to other material http://stackoverflow.com/questions/15542608/design-patterns-exception-error-handling – Richard Chambers Oct 22 '13 at 12:24

4 Answers4

6

You have to adhere to the single responsibility principle: every method does one thing. Right now your methods do two things: check if it's vertical/horizontal and calculate something.

Another note here: don't use exceptions for program flow.

You should split it up to something like this:

bool isVertical(parameters){}
bool isHorizontal(parameters){}
SomeClass CalculateVertical(parameters){}
SomeClass CalculateHorizontal(parameters){}

Your program flow could look like this:

if(isVertical(something)){
 CalculateVertical(something);
else if (isHorizontal(something)){
 CalculateHorizontal(something);
}

Example implementation:

SomeClass CalculateVertical(something){
 if(!isVertical(something)) { throw new IllegalArgumentException() }
 // Calculations
}

Note that this exception doesn't have to be caught by the programmer.

Jeroen Vannevel
  • 43,651
  • 22
  • 107
  • 170
  • 1
    Agreed. I would be lazy and just throw IllegalArgumentException. It's an existing Exception and it's unchecked. It would force the user to validate input before attempting to call the methods. – Kayaman Oct 22 '13 at 12:20
  • So should I assume in CalculateVertical that I've definitely been given a vertical line, and not have any checking in that method? – Emma Oct 22 '13 at 12:20
  • @Emma: you should never trust the user. I would call `isVertical` again in `CalculateVertical` and throw an exception (like `IllegalArgumentException` as Kayaman says) if it returns `false`. – Jeroen Vannevel Oct 22 '13 at 12:22
  • @Emma You can again check isVertical in the CalculateVertical method – Barun Oct 22 '13 at 12:31
  • But then, if I'm throwing an exception from CalculateVertical, I'll have to use try catch in pointsInWindow anyway - so then I'm back at the same point as I was before, just with an extra call to isVertical in there. Unless I've misunderstood something? – Emma Oct 22 '13 at 12:31
  • You don't have to catch `IllegalArgumentException`, it's an unchecked exception. It's the responsibility of the programmer to call the `isVertical` method first, you're just performing a final check. – Jeroen Vannevel Oct 22 '13 at 12:35
  • Ok, thank you! So the final solution is to perform my own checks, without using exceptions, and as a final resort throw an unchecked exception. Will do! And apologies for being so slow to get it, you've been very helpful. – Emma Oct 22 '13 at 12:44
  • That's exactly the solution, yes :) It's okay, we've all been there. – Jeroen Vannevel Oct 22 '13 at 12:45
1

or you can modify like below:

pointsInWindow (int xMin, int xMax, int yMin, int yMax) {
    try {
        // Find list of all the points between xMin and xMax.
        // Remove from list all points not between yMin and yMax
    }
    catch (LineIsVerticalException e) {
        // If line is vertical, cannot find points between xMin and xMax
        // do something..
        }
    catch (LineIsHorizontalException e) {
        // unless LineIsVerticalException is superclass of LineIsHorizontalException,
        // this will work 
        // do something ..
        }
    }
}
Raúl
  • 1,542
  • 4
  • 24
  • 37
1

In general I try to use Exceptions for unexpected problems (e.g. network failure) as opposed to just border cases. It is however a blurry distinction and depends on your specific application context.

Specifically for your problem. What about splitting the functionality. Make two functions that detect whether or not a given line is horizontal or vertical (e.g. boolean isVertical());.

In your pointsInWindow function you could then check first whether you are dealing with the special case of a vertical/horizontal line and if not call further to the findPointsInRange methods.

Try never to duplicate logic as this violates the DRY principle DRY principle and tends to cause problems further on when maintaing your code.

Hope this helps, Markus

markus
  • 1,631
  • 2
  • 17
  • 31
-1
findPointsInRangeOfX (int xMin, int xMax) throws LineIsVerticalException {
    if(isLineVeritical(xMin, xMax)){
        throw new LineIsVerticalException();
    }

    return calculateValuesBetweenMinAndMax(xMin, xMax);
}
Atul Jain
  • 1,035
  • 2
  • 16
  • 24