Hey Michael: It's safe to simply use getParameter(). Per JDK docs ( http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/ServletRequest.html#getParameter(java.lang.String))<http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/ServletRequest.html#getParameter(java.lang.String)>
"If you use this method with a multivalued parameter, the value returned is equal to the first value in the array returned by getParameterValues." So I'd just go ahead with that simplification. Likewise with the callback param. Let me know when that's done and I'll do a final review. Thanks, John On Tue, Oct 7, 2008 at 1:30 PM, <[EMAIL PROTECTED]> wrote: > 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 >
