http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right):
http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode38 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:38: import org.apache.commons.lang.RandomStringUtils; Remove unused import. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode119 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:119: List<Uri> batchUris = Lists.newArrayList(); Add a comment here indicating that batchUris holds uris for the current batch of uris being concatenated. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode120 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:120: List<Uri> uris = Lists.newArrayList(); Add a comment before this line indicating that uris holds the concatenated uris formed from batches which satisfy the GET URL limit constraint. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (left): http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#oldcode124 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:124: ImmutableList.of(RESOURCES_ONE, RESOURCES_TWO, RESOURCES_ONE); As discussed offline, let us retain RESOURCES_ONE and do the necessary checks. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode55 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:55: Remove inserted tab? http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode134 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:134: assertFalse(jsonParams.contains(json)); Maybe assert that jsonParams begins with json. http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode314 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:314: Uri urlA = Uri.parse(generateUrl(ConcatUriManager.ConcatData.getMaxUrlLength() / 2)); A more generic test would be one where we deal with atleast 3 URLs (with the second one being slightly shorter). And 2 of these urls get concatenated into one concat-uri and the 3rd one moves to the 2nd concat-uri. Could we modify both the tests here for the url limit, to accommodate this? http://codereview.appspot.com/3734041/diff/137001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode332 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:332: assertNull(concatUris.get(0).getUris().get(0).getQueryParameter("2")); I think we should do the usual set of asserts that we always do in other tests (see block 144 - 158). If convenient, please move these asserts to a helper routine to help simplify your code. http://codereview.appspot.com/3734041/
