52

I am writing code to find and intersection of 2 lines. When slopes of the lines are equal they dont intersect. But on the other hand an input with slopes of equal value is completely valid.

public static Point calculateIntersection(Line line1, Line line2) {

    if (line1 == null || line2 == null) {
        throw new NullPointerException(" some message ");
    }

    if (line1.getConstant() == line2.getConstant()) {
        return new Point(0, line1.getConstant());
    }

    if (line1.getSlope() == line2.getSlope()) {
        throw new IllegalArgumentException("slopes are same, the lines do not intersect.");
    }

    int x = (line2.getConstant() - line1.getConstant()) / (line1.getSlope() - line2.getSlope());
    int y = line1.getSlope() * x + line1.getConstant();

    return new Point(x, y);
}

The question is throwing illegal argument exception the right thing to do ? Since input is valid, it does not completely convince me.

Is custom exception the right thing to do ? Sounds like a good choice but an additional opinion would help.

Thanks

JavaDeveloper
  • 5,320
  • 16
  • 79
  • 132
  • 3
    Is this really an exception? I'm all for preferring exceptions over nullable return values but in this case it would express an expected return value. – Matt Whipple Jun 10 '13 at 01:57
  • Input Mismatch Exception, Number Format Exception or in the most generic case, just add Illegal Argument Exception. – Radioactive Oct 26 '18 at 16:01

6 Answers6

56

The question is is throwing illegal argument exception the right thing to do?

There is no single "right thing to do". It depends on how you want / need to "frame" this condition; i.e. is it a bug, a user input error, or something that the program is supposed to be able to deal with?

  • If the case of two lines not intersecting is unequivocally a "bug", then IllegalArgumentException is fine. This is what the exception is designed for. (Note that it is an unchecked exception, so the expectation is that it won't be caught / recovered from.)

  • If this is a case that you expect the program to be able to recover from by itself, then a custom exception is the best idea. That way, you reduce the possibility of your code getting confused by (say) a library method throwing (say) an IllegalArgumentException ... than means something other than "two lines intersected".

  • If this case is something that you expect to report to the end user as part of input validation, then a generic "validation error" exception might be more appropriate than a specific custom exception. However, this method doesn't look like it is designed to be used (solely) for user input validation.


In some contexts, it may be better to not throw an exception at all, but (IMO) this is not one of those contexts.

The alternatives to throwing an exception are returning null or returning a Point value that means "no such point" to the calling code. The problems with these alternatives are:

  • If you return null the application has to deal with the null case ... or there will be NPEs.
  • There is no natural Point instance that could be used to mean "not a point"1.

This is not to say that you couldn't make these alternatives work. It is just that in this context, it will probably be more work to do that, and probably there won't be a tangible pay-off.


1 - I am assuming Point is java.awt.Point or similar. Obviously you could define and use a custom Point class which provides a "no such point" instance. But it will come at a cost. And you will need to deal with the case where code accidentally uses the "no such point" instance in some computation. And you are probably back where you started; i.e. throwing an exception!

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
4

This almost certainly should not throw an exception, because it makes perfect sense to invoke a method like this with any two Line values. You have already appropriately handled null values.

You have also, very reasonably, defined the behavior of your class in one of the ill-defined input situations, namely two coincident "constant" (horizontal) lines, where you return the point at x=0 on that line. You ought to similarly select return values for the other cases of ill-defined inputs: coinciding vertical lines, coinciding lines that are neither horizontal nor vertical, and non-coinciding parallel lines.

In my opinion, the most natural result for the last case - non-coinciding parallel lines - would be null, reflecting the fact that there is no point of intersection.

It would then be up to the client to decide whether a null intersection warrants an exception, an error message, or whatever. E.g. an interactive shell prompting the user for lines to intersect would probably print an error message and ask the user to try again. Some more complicated calculation, e.g. a linear optimizer trying to define boundaries for its search, might want to throw IllegalArgumentException if the constraints giving rise to the parallel lines contradict each other.

Of course, the return values in all these cases (coincident lines or non-coincident parallel lines) should be precisely documented in the method's javadoc.

Andy Lowry
  • 785
  • 7
  • 12
2

I'd say you do the right thing: you catch the condition early. It is either that or people will complain that "your program is buggy, look at this input data, division by 0".

Given that there will be no such error in 99+% of situations, this is an exceptional condition and doesn't grant declaring a checked exception, so an unchecked exception looks indeed like the right choice.

Now, as to whether IllegalArgumentException is the "good one", it is at least the closest exception describing this case... You can, if you feel you have a better name, always create your own one inheriting RuntimeException.

IF, on the other hand, this situation is not that infrequent, then maybe the logic to reach that function should be reviewed so as to never hit this condition in the first place.

Jared Burrows
  • 54,294
  • 25
  • 151
  • 185
fge
  • 119,121
  • 33
  • 254
  • 329
2

As @Andy-Lowry and @KamikazeCZ say, this should not be an exception.

This method should not concern itself with whether the client expects lines to always intersect or not; it should only bother with finding the intersection of two lines -- something which inherently might not happen.

If the caller gets back a result indicating no-intersection, then THAT code can decide whether it's invalid-input because the end-user was duly warned, or something they can handle (perhaps by re-prompting), or throw a customized exception.

So, back to what should this method return? Some sort of sentinel value, the same way that indexOf returns -1 in the collections library. Returning null is a reasonable sentinel. In Java 8, you can return an Optional<Point>, to help remind the caller that there might not be a proper Point.

You have an additional problem, too: what somebody asks for the intersection of a line with itself? (Mathematically, the intersection of two lines is either 0 point, 1 point, or infinitely many points.) You might need to be able to return two sentinel values, which is more involved, in Java. This method could weasel out of the situation this time, by saying "in the case of multiple answers, this method may return any of them", or (what I'd probably do) "...returns the Point nearest the origin".

Btw, this thinking largely follows from a unit-test mindset: start by defining the correct answer should be for a variety of corner-case inputs, before launching into code and kinda committing yourself to a certain return-type etc.

Finally: when comparing the results of getSlope() using ==, beware floating-point roundoff errors. It might be the best thing to do here, but it's still problematic. The way you assume (or round) the intersection to ints, though, suggests you might have very special constraints/assumptions going on in your problem.

not-just-yeti
  • 17,673
  • 1
  • 18
  • 15
-2

Exceptions should be used to catch errors in program flow ("what happens inside") , not for input validation. I wouldn't throw an exception at all. Think about it, this is not what "exception" means as it is completely normal for the user to input two lines with equal slopes.

KamikazeCZ
  • 714
  • 8
  • 22
  • Look at the next line: this is either that or a division by 0 – fge Jun 10 '13 at 02:00
  • 2
    When the user enters two same slopes, of course you won't continue and divide by zero, you'll warn him and stop instead. Throwing an exception is not a good idea because it... well... isn't an exception. It's completely normal. This is what the if-statement is for. – KamikazeCZ Jun 10 '13 at 02:04
  • Oh yeah? The user expects a _result_. And the result of an intersection. – fge Jun 10 '13 at 02:05
  • 2
    The user expects a result or, if you can't give a result, the error message that he understands. Throwing an exception: 1) doesn't correspond to what exceptions are made for 2) might not be as readable 3) includes some overhead, which is not so bad in this case but it can be in some others – KamikazeCZ Jun 10 '13 at 02:09