LGTM
I see common code in the manager and the concat manager.
I think you can create a base manager for that code.



http://codereview.appspot.com/217110/diff/3009/3010
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
(right):

http://codereview.appspot.com/217110/diff/3009/3010#newcode50
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:50:
*
http://www.example.com/gadgets/proxy/refresh=1&.../http://www.foo.com/img.gif
Add the start and end section markers in the example

http://codereview.appspot.com/217110/diff/3009/3010#newcode120
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:120:
}
Maybe put common code in basic manager: getBaseUriBuilder()

http://codereview.appspot.com/217110/diff/3009/3010#newcode227
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:227:
private String getReqConfig(String container, String key) {
This seems like a repeated code, maybe create a base uri manager.

http://codereview.appspot.com/217110/diff/3009/3012
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
(right):

http://codereview.appspot.com/217110/diff/3009/3012#newcode79
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:79:
String path = "/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN +
"/path";
Test also the case that CHAINED_PARAM_TOKEN is at the end of the path.

http://codereview.appspot.com/217110/show

Reply via email to