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); > + } > + } > } > > >
