Comments inlined.
http://codereview.appspot.com/7259/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java (right): http://codereview.appspot.com/7259/diff/1/3#newcode46 Line 46: static final String GET_REQUEST_CALLBACK_PATTERN = "[A-Za-z\\.]+"; On 2008/10/07 00:56:07, johnfargo wrote:
This should be a java.util.regex.Pattern to avoid recompiling it on
every match. Done. Also replaced consumer with validPattern.matcher(result).matches(). http://codereview.appspot.com/7259/diff/1/3#newcode46 Line 46: static final String GET_REQUEST_CALLBACK_PATTERN = "[A-Za-z\\.]+"; On 2008/10/07 04:12:59, etnu00 wrote:
Underscores need to be supported as well.
Done. Added to test. Thinking more about this, does it need to support 0-9 as well? http://codereview.appspot.com/7259/diff/1/3#newcode61 Line 61: HttpServletResponse response) throws IOException { On 2008/10/07 00:56:07, johnfargo wrote:
Same arg indentation comment as below.
Done. http://codereview.appspot.com/7259/diff/1/3#newcode67 Line 67: request, GET_REQUEST_REQ_PARAM, null); On 2008/10/07 00:56:07, johnfargo wrote:
do all these fit in 100 chars?
Done. Fixed. http://codereview.appspot.com/7259/diff/1/3#newcode121 Line 121: String validPattern) throws On 2008/10/07 00:56:07, johnfargo wrote:
Shindig common style is to put as many args as fit in 100 chars on the
first
line, then wrap w/ 4-char indent.
Done. http://codereview.appspot.com/7259/diff/1/3#newcode126 Line 126: String[] values = request.getParameterValues(parameter); On 2008/10/07 00:56:07, johnfargo wrote:
request.getParameter() does the values[0] piece for you - using it
would
simplify the code (and test) significantly
I thought of that too, but I am thinking of cases when someone specifies multiple values, ie: req=...&req=... In which case, we can just take the first one, and proceeds (with/without warning). Or, like here, we can break and make the misuse explicit to the user. I am probably over-checking here. Same case with parameter. callback. What do you think? http://codereview.appspot.com/7259/diff/1/3#newcode133 Line 133: values[0], getRequestCharacterEncoding(request)); On 2008/10/07 04:12:59, etnu00 wrote:
This isn't necessary. getParameter will automatically decode a url
encoded
parameter value.
Done. Removed. http://codereview.appspot.com/7259/diff/1/3#newcode136 Line 136: parameter + "' specified. Expected: " + validPattern); On 2008/10/07 00:56:07, johnfargo wrote:
You could just use IllegalArgumentException here and save some code.
Done. Good idea. http://codereview.appspot.com/7259/diff/1/3#newcode139 Line 139: return result; On 2008/10/07 00:56:07, johnfargo wrote:
Since this is used for only one argument, it seems to me that you
could probably
just inline the appropriate code into doGet()
Done. Inline the pattern check for callback, if that's what you mean. http://codereview.appspot.com/7259/diff/1/3#newcode145 Line 145: byte[] body) { On 2008/10/07 00:56:07, johnfargo wrote:
same
Done. Extends through line. http://codereview.appspot.com/7259/diff/1/3#newcode176 Line 176: private class ParameterException extends Exception { On 2008/10/07 04:12:59, etnu00 wrote:
This checked exception isn't adding much value here. As John says, an IllegalArgumentException is more fitting for the inputs.
Done. Replaced with IllegalArgumentException. http://codereview.appspot.com/7259/diff/1/3#newcode182 Line 182: private class Result { On 2008/10/07 04:12:59, etnu00 wrote:
This should be a static class.
Done. http://codereview.appspot.com/7259/diff/1/2 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/RpcServletTest.java (right): http://codereview.appspot.com/7259/diff/1/2#newcode35 Line 35: public class RpcServletTest extends ServletTestFixture { On 2008/10/07 04:12:59, etnu00 wrote:
Please don't write any new code depending on ServletTestFixture. It's deprecated. New tests should use junit4 conventions. See other tests for examples.
Done. Now extends TestCase. http://codereview.appspot.com/7259/diff/1/2#newcode39 Line 39: private class TestJsonRpcHandler extends JsonRpcHandler { On 2008/10/07 04:12:59, etnu00 wrote:
inner classes should come after test methods.
Got rid of this inner class entirely. Replaced with EasyMock. http://codereview.appspot.com/7259/diff/1/2#newcode47 Line 47: super(executor, processor, urlGenerator); On 2008/10/07 04:12:59, etnu00 wrote:
None of these are used. Passing nulls to the parent ctor or simply
mocking a
JsonRpcHandler is strongly preferred here.
Same, replaced this inner class completely with EasyMock. http://codereview.appspot.com/7259/diff/1/2#newcode48 Line 48: } On 2008/10/07 00:56:07, johnfargo wrote:
No need for this ctor. I'd suggest replacing it with helper methods: void expectNormalRequest(String req, String resp) void expectRpcException(RpcException) void expectJsonException(JsonException) ...which reinitialize fields as appropriate.
Replaced with EasyMock expects(). http://codereview.appspot.com/7259/diff/1/2#newcode71 Line 71: private void setupGet(String[] reqParamValues, String[] callbackParamValues) { On 2008/10/07 00:56:07, johnfargo wrote:
I'd suggest having this as a helper method that returns an
HttpServletRequest
rather than relying on it having that side effect. Then each of your
tests would
look pretty simple: HttpServletRequest req = makeRequestObj(req, callback); handler.set[Expectation(...)] servlet.doGet(req, recorder) assertX() if appropriate, or fail() if exception expected
I think I got it. http://codereview.appspot.com/7259/diff/1/2#newcode151 Line 151: String[] reqParamValues, On 2008/10/07 04:12:59, etnu00 wrote:
You don't need to bother testing multiple parameters being passed. The
handling
of getParameter is well defined as part of the HTTP standard.
Can we assume that if multiple parameters (ex: req=...&req=...) are specified, then only the very first parameter is consumed (and the others are dropped), without any (warning?) message to the user? http://codereview.appspot.com/7259
