PlaceNames to routes in Gwtp

[philippe.beaudoin, 2012-05-14 16:09:39]

Do we need gwtp-samples/.DS_Store ? It's empty. Can we remove it from the repo?

Some basic architectureal comment. I just reviewed a few files. I really like
what you're doing here and wonder if we could pull some of this in GWTP's core.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
File
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java
(right):

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:86:
// If the history length hasn't changed, then pushState mustn't be enabled. 
Fall back to $wnd.location.
Line too long.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:135:
placeManager.@com.gwtplatform.mvp.client.proxy.Html5PushStatePlaceManager::onPopState(Ljava/lang/String;)(event.state.historyToken);
Line too long.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
File
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java
(right):

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:27:
* than server side routes you might know from Play, JAX-WS or Play.
Nits:
- same as server side...
- Play mentioned twice. Did you mean playn?

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:33:
* <li>/user/{id}/dashboard</li>
Twice the same example?

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:45:
* @author <a href="http://www.raphaelbauer.com">Raphael Bauer</a>
Make sure both are in the POMs (and on the Wiki?)

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:49:
private final Map<String, String> params = new HashMap<String, String>();
params is already defined in PlaceRequest. We don't want it duplicated (make it
protected in the parent class, I think it's fine in this case).

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:67:
private interface PlaceTokenPart {
I'd like some documentation for this. Given the methods it's not 100% clear what
this is doing.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:69:
* Does this part match the given part.
Nit: Checks that this part matches ...

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:82:
private static class ParameterizedPlaceTokenPart implements PlaceTokenPart {
Again, documentation with an example would be great.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:89:
public String getParameterName() {
@Override

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:94:
public boolean matches(String part) {
Matches anything!? Sounds counter-intuitive. Would need some doc.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:99:
private static class StaticPlaceTokenPart implements PlaceTokenPart {
Again, documentation with an example would be great.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:119:
if (nameToken.equals(getNameToken())) {
What happens for a parameterized name token? Does getNameToken() return null?
The empty string? Is there a danger that nameToken be an empty string and match
where it shouldn't?

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:135:
pathParams.put(parameterName, parts[i]);
The pathParams variable is puilt out of the content of getNameToken() every time
matchesNameToken is called, however nameToken is a final member variable.
Therefore all the logic in that method is invoked too often.

I suggest you refactor to store
  final String[] parts
in ParameterizedPlaceRequest and to build pathParams and params right in the
constructor.

http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:157:
}
Since getParameter() looks at both params and pathParams shouldn't
getParameterNames return the union of params and pathParams?

[raphael.andre.bauer, 2012-05-22 12:43:02]

Hi Philippe,


thanks for taking the time and reviewing our changes :)

Should I already move our changes into gwtp-mvp-client?
html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStateManager.java
html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java
html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedParameterTokenFormatter.java

You'll get the augmented version asap...


Cheers,


Raphael






On Mon, May 14, 2012 at 6:09 PM,  <philippe.beaudoin@gmail.com> wrote:
> Do we need gwtp-samples/.DS_Store ? It's empty. Can we remove it from
> the repo?
>
> Some basic architectureal comment. I just reviewed a few files. I really
> like what you're doing here and wonder if we could pull some of this in
> GWTP's core.
>
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
> File
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java
> (right):
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:86:
> // If the history length hasn't changed, then pushState mustn't be
> enabled.  Fall back to $wnd.location.
> Line too long.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:135:
>
placeManager.@com.gwtplatform.mvp.client.proxy.Html5PushStatePlaceManager::onPopState(Ljava/lang/String;)(event.state.historyToken);
> Line too long.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
> File
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java
> (right):
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:27:
> * than server side routes you might know from Play, JAX-WS or Play.
> Nits:
> - same as server side...
> - Play mentioned twice. Did you mean playn?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:33:
> * <li>/user/{id}/dashboard</li>
> Twice the same example?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:45:
> * @author <a href="http://www.raphaelbauer.com">Raphael Bauer</a>
> Make sure both are in the POMs (and on the Wiki?)
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:49:
> private final Map<String, String> params = new HashMap<String,
> String>();
> params is already defined in PlaceRequest. We don't want it duplicated
> (make it protected in the parent class, I think it's fine in this case).
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:67:
> private interface PlaceTokenPart {
> I'd like some documentation for this. Given the methods it's not 100%
> clear what this is doing.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:69:
> * Does this part match the given part.
> Nit: Checks that this part matches ...
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:82:
> private static class ParameterizedPlaceTokenPart implements
> PlaceTokenPart {
> Again, documentation with an example would be great.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:89:
> public String getParameterName() {
> @Override
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:94:
> public boolean matches(String part) {
> Matches anything!? Sounds counter-intuitive. Would need some doc.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:99:
> private static class StaticPlaceTokenPart implements PlaceTokenPart {
> Again, documentation with an example would be great.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:119:
> if (nameToken.equals(getNameToken())) {
> What happens for a parameterized name token? Does getNameToken() return
> null? The empty string? Is there a danger that nameToken be an empty
> string and match where it shouldn't?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:135:
> pathParams.put(parameterName, parts[i]);
> The pathParams variable is puilt out of the content of getNameToken()
> every time matchesNameToken is called, however nameToken is a final
> member variable. Therefore all the logic in that method is invoked too
> often.
>
> I suggest you refactor to store
>  final String[] parts
> in ParameterizedPlaceRequest and to build pathParams and params right in
> the constructor.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:157:
> }
> Since getParameter() looks at both params and pathParams shouldn't
> getParameterNames return the union of params and pathParams?
>
> http://codereview.appspot.com/6213044/



-- 
inc: http://ars-machina.raphaelbauer.com
tech: http://ars-codia.raphaelbauer.com
web: http://raphaelbauer.com

[raphael.andre.bauer, 2012-05-25 09:42:17]

Hi Philippe,


not sure if this is the correct way to do it in the
http://codereview.appspot.com style.

I did some more major changes and moved the classes into their correct
location under com.gwtplatform.mvp.client.proxy

I also added a lot of comments as requested by your earlier review and
fixed all issues of the "old" code review.

New codereview is at:
http://codereview.appspot.com/6257054/


Feedback heavily appreciated :)


Cheers,


Raphael

On Mon, May 14, 2012 at 6:09 PM,  <philippe.beaudoin@gmail.com> wrote:
> Do we need gwtp-samples/.DS_Store ? It's empty. Can we remove it from
> the repo?
>
> Some basic architectureal comment. I just reviewed a few files. I really
> like what you're doing here and wonder if we could pull some of this in
> GWTP's core.
>
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
> File
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java
> (right):
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:86:
> // If the history length hasn't changed, then pushState mustn't be
> enabled.  Fall back to $wnd.location.
> Line too long.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/Html5PushStatePlaceManager.java:135:
>
placeManager.@com.gwtplatform.mvp.client.proxy.Html5PushStatePlaceManager::onPopState(Ljava/lang/String;)(event.state.historyToken);
> Line too long.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
> File
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java
> (right):
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:27:
> * than server side routes you might know from Play, JAX-WS or Play.
> Nits:
> - same as server side...
> - Play mentioned twice. Did you mean playn?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:33:
> * <li>/user/{id}/dashboard</li>
> Twice the same example?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:45:
> * @author <a href="http://www.raphaelbauer.com">Raphael Bauer</a>
> Make sure both are in the POMs (and on the Wiki?)
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:49:
> private final Map<String, String> params = new HashMap<String,
> String>();
> params is already defined in PlaceRequest. We don't want it duplicated
> (make it protected in the parent class, I think it's fine in this case).
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:67:
> private interface PlaceTokenPart {
> I'd like some documentation for this. Given the methods it's not 100%
> clear what this is doing.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:69:
> * Does this part match the given part.
> Nit: Checks that this part matches ...
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:82:
> private static class ParameterizedPlaceTokenPart implements
> PlaceTokenPart {
> Again, documentation with an example would be great.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:89:
> public String getParameterName() {
> @Override
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:94:
> public boolean matches(String part) {
> Matches anything!? Sounds counter-intuitive. Would need some doc.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:99:
> private static class StaticPlaceTokenPart implements PlaceTokenPart {
> Again, documentation with an example would be great.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:119:
> if (nameToken.equals(getNameToken())) {
> What happens for a parameterized name token? Does getNameToken() return
> null? The empty string? Is there a danger that nameToken be an empty
> string and match where it shouldn't?
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:135:
> pathParams.put(parameterName, parts[i]);
> The pathParams variable is puilt out of the content of getNameToken()
> every time matchesNameToken is called, however nameToken is a final
> member variable. Therefore all the logic in that method is invoked too
> often.
>
> I suggest you refactor to store
>  final String[] parts
> in ParameterizedPlaceRequest and to build pathParams and params right in
> the constructor.
>
>
http://codereview.appspot.com/6213044/diff/1/gwtp-samples/gwtp-routes-and-htm...
>
gwtp-samples/gwtp-routes-and-html5pushstate/src/main/java/com/gwtplatform/mvp/client/proxy/ParameterizedPlaceRequest.java:157:
> }
> Since getParameter() looks at both params and pathParams shouldn't
> getParameterNames return the union of params and pathParams?
>
> http://codereview.appspot.com/6213044/



-- 
inc: http://ars-machina.raphaelbauer.com
tech: http://ars-codia.raphaelbauer.com
web: http://raphaelbauer.com