Addressed your comments.
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];
On 2010/03/18 21:04:07, Ray Ryan wrote:
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() } )
Done.
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 {
This will need to exist in some form. Will add a unit test shortly.
Currently, just a TODO.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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.
On 2010/03/18 21:04:07, Ray Ryan wrote:
I'd snip this comment, it reads like a TODO that you've just TODone
Done.
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.
Yeah, removed the TODO.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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());
On 2010/03/18 21:04:07, Ray Ryan wrote:
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.
Done.
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>
On 2010/03/18 21:04:07, Ray Ryan wrote:
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?
Done.
http://gwt-code-reviews.appspot.com/240801/diff/1/5#newcode93
bikeshed/src/com/google/gwt/sample/expenses/server/ExpensesDataServlet.java:93:
e.printStackTrace();
True. done.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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) {
On 2010/03/18 21:04:07, Ray Ryan wrote:
Which means you could lose this catch entirely
Done.
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();
True. It is currently not used. Updated the code.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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
yes, make it more prominent. Changed todo to reflect that.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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)
Wouldn't the read-only properties be final? the method returns true in
that case.
On 2010/03/18 21:04:07, Ray Ryan wrote:
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.
