Comments below. Plus, could a TODO to MethodName.java saying that the plan is to eliminate it in preference to reflecting on the tool-generated RequestFactory interface for this info?
http://gwt-code-reviews.appspot.com/240801/diff/1/3 File bikeshed/src/com/google/gwt/sample/expenses/gen/ReportRequestImpl.java (right): http://gwt-code-reviews.appspot.com/240801/diff/1/3#newcode63 bikeshed/src/com/google/gwt/sample/expenses/gen/ReportRequestImpl.java:63: Object values[] = new Object[1]; Confusing name, since the nested methods below also create objects named values. Might even be a checkstyle violation. Can you rename it? Actually, might as well just inline it: UrlParameterManager.getUrlFragment(new Object[] { id.getEntity().getId() } ) http://gwt-code-reviews.appspot.com/240801/diff/1/4 File bikeshed/src/com/google/gwt/sample/expenses/gen/UrlParameterManager.java (right): http://gwt-code-reviews.appspot.com/240801/diff/1/4#newcode23 bikeshed/src/com/google/gwt/sample/expenses/gen/UrlParameterManager.java:23: public class UrlParameterManager { Is this class a keeper? If so, it should get a unit test now. (If not, I wouldn't bother.) http://gwt-code-reviews.appspot.com/240801/diff/1/5 File bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java (right): http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode65 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:65: // use reflection code here. I'd snip this comment, it reads like a TODO that you've just TODone http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode71 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:71: // TODO: check if method names must be unique in a class. Of course not. That's the kind of thing that I think will actually get easier when you turn the RequestFactory interface into your config, you'll have all the client and server side params at your fingertips. http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode85 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:85: assert Modifier.isStatic(methodOperation.getModifiers()); Please don't use asserts in server side code. Make this a real check, and throw an IllegalArgumentException if it fails. Also, should do this as soon as you get methodOperation, not a few lines later. http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode86 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:86: // the return value is a List<? extends Entity> Why aren't you doing the cast here? Then you can clean up the signature of your getJsonArray method. Also, the Entity interface is an implementation detail of my mock world (which is why Entity was private). There is no comparable interface we can rely on in real life. For that matter, I don't think JPA persistence is going to rely on id and version properties having those precise names, nor having accessor methods. I expect we'll need to be looking reflectively at the fields of the server side objects, for the javax.persistence.Id and javax.persistence.Version fields, and grab them directly. For now, if you treat VERSION and ID as always being in the requested set (which you do already), do you even need to know about the Entity interface? Perhaps this comment should become a TODO, down in getJsonArray? http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode93 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:93: e.printStackTrace(); Won't all these printStackTrace calls just result in the stack getting printed twice, here and when jetty or whatever catches them later? http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode96 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:96: } catch (IllegalArgumentException e) { Which means you could lose this catch entirely http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode164 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:164: Class<? extends Entity> entityClass = firstElement.getClass(); This looks like the only reason that Entity is public, and you don't seem to use the knowledge anywhere. http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode243 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:243: * TODO: fix hacks. 1. encoding long as String 2. encoding Date as Double What's to fix? This is the plan. The hack is that we need to make it a bit more intentional, tied to the annotations in the RequestFactory interface. Is that your point here? http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode260 bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:260: if (!Modifier.isStatic(modifiers) || !Modifier.isFinal(modifiers) why not final? Won't this prevent requests for read-only properties? http://gwt-code-reviews.appspot.com/240801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
