search

dfabulich / sitemapgen4j

SitemapGen4j is a library to generate XML sitemaps in Java.
Apache License 2.0
160 stars 90 forks source link

Added checked exceptions for InvalidURLException #9

Closed jamesbrink closed 9 years ago

jamesbrink commented 9 years ago

Hello, I really like this library, but there are far to many unchecked exceptions. This is just a small start for migrating some of the Exceptions. This breaks backwards compatibility, but I feel its important that a library not be filled with so many unchecked/runtime Exceptions. Thoughts?

mkurz commented 9 years ago

/cc @dfabulich

dfabulich commented 9 years ago

I'm closing this PR. Certainly it breaks backwards compatibility, but I also don't think it's a good idea.

I'm of the opinion that checked exceptions need to be used sparingly. Here's a fun discussion about them. http://stackoverflow.com/questions/613954/the-case-against-checked-exceptions

Specifically, checked exceptions should never be used to notify the developer that they have a bug in their code, but should instead be used to notify the developer that something outside the code is unexpected. FileNotFoundException is a good checked exception.

My claim is that the InvalidURLException you propose is telling developers about a bug in their code. You shouldn't handle an InvalidURLException, you change your code so that you don't pass mismatching URLs to the API. Mismatching URLs aren't something that just happens to you, like a missing file.

Having said that, there are a couple of dozen places where RuntimeException is thrown in this library, and I'd be OK with replacing them with more specific unchecked exceptions. e.g. instead of throwing RuntimeException in UrlUtils, we'd throw an InvalidURLException that extends RuntimeException. (Or, better, BaseUrlMismatchException.)