Sure, added another comment, this time to the exception itself. Cheers, John
On Thu, Feb 18, 2010 at 10:00 AM, Ziv Horesh <[email protected]> wrote: > LGTM > > If I understand correctly, the ultimate goal is to have a non > RuntimeException here. > The reason for RuntimeException now, is so we can change in stages. > > Maybe add some explanation in the class doc, of what the plan, and who need > to use it. > > > > On Tue, Feb 16, 2010 at 4:10 PM, <[email protected]> wrote: > >> Reviewers: shindig.remailer_gmail.com, >> >> Description: >> This CL introduces a trivial wrapper UriException class, which wraps >> IllegalArgumentException (itself a RuntimeException), and changes the >> Uri class to throw it exclusively rather than IllegalArgumentException >> or RuntimeException directly. >> >> This allows calling code to wrap relevant methods in >> semantically-bounded try/catch blocks without our having to have Uri's >> methods throw a checked exception, which in turn would necessitate large >> numbers of downstream changes. >> >> In this way, we can slowly phase in use of UriException, to the point >> that eventually we may find it relatively easy to change this exception >> to a checked version of itself with minimal trouble. >> >> Comments welcome. >> >> Please review this at http://codereview.appspot.com/210041/show >> >> Affected files: >> java/common/src/main/java/org/apache/shindig/common/uri/Uri.java >> >> >> Index: java/common/src/main/java/org/apache/shindig/common/uri/Uri.java >> =================================================================== >> --- java/common/src/main/java/org/apache/shindig/common/uri/Uri.java >> (revision 910762) >> +++ java/common/src/main/java/org/apache/shindig/common/uri/Uri.java >> (working copy) >> @@ -97,7 +97,17 @@ >> * @return A new Uri, parsed into components. >> */ >> public static Uri parse(String text) { >> - return parser.parse(text); >> + try { >> + return parser.parse(text); >> + } catch (IllegalArgumentException e) { >> + // This occurs all the time. Wrap the exception in a Uri-specific >> + // exception, yet one that remains a RuntimeException, so that >> + // callers may catch a specific exception rather than a blanket >> + // Exception, as a compromise between throwing a checked exception >> + // here (forcing wide-scale refactoring across the code base) and >> + // forcing users to simply catch abstract Exceptions here and >> there. >> + throw new UriException(e); >> + } >> } >> >> /** >> @@ -105,7 +115,7 @@ >> */ >> public static Uri fromJavaUri(URI uri) { >> if (uri.isOpaque()) { >> - throw new IllegalArgumentException("No support for opaque Uris " + >> uri.toString()); >> + throw new UriException("No support for opaque Uris " + >> uri.toString()); >> } >> return new UriBuilder() >> .setScheme(uri.getScheme()) >> @@ -124,7 +134,7 @@ >> return new URI(toString()); >> } catch (URISyntaxException e) { >> // Shouldn't ever happen. >> - throw new IllegalArgumentException(e); >> + throw new UriException(e); >> } >> } >> >> @@ -185,7 +195,7 @@ >> if (StringUtils.isEmpty(uri.authority) && >> StringUtils.isEmpty(uri.path) && >> StringUtils.isEmpty(uri.query)) { >> - throw new IllegalArgumentException("Invalid scheme-specific part"); >> + throw new UriException("Invalid scheme-specific part"); >> } >> } >> >> @@ -387,4 +397,14 @@ >> if (!(obj instanceof Uri)) {return false;} >> return Objects.equal(text, ((Uri)obj).text); >> } >> + >> + public static class UriException extends IllegalArgumentException { >> + private UriException(Exception e) { >> + super(e); >> + } >> + >> + private UriException(String msg) { >> + super(msg); >> + } >> + } >> } >> >> >> >
