2012/10/2 Marko Strukelj <span dir="ltr"><<a href="mailto:mstrukel@redhat.com" target="_blank">mstrukel@redhat.com</a>></span><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Some comments ... (I haven't looked at this that closely before so there are some things here that relate to much earlier commits).<br>
<br>
- Logging seems to be the only android specific thing in the library ATM. I'm thinking that maybe we could treat it as a generic java client library using slf4j maybe, and provide pluggability to wire it up to android.util.Log on Android.<br>
</blockquote><div><br></div><div>I don't know. I need to think about this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I think Matthias noticed that the API is nicely aligned with the one for iOS, which is exactly what we want. (I didn't compare, so I can't say :)<br>
<br>
- Looking at HttpRestProvider get() method doesn't take an id - it's an operation that retrieves a list of all instances - presuming URL points to something like <a href="http://host/task" target="_blank">http://host/task</a>, and not to something like <a href="http://host/task/100" target="_blank">http://host/task/100</a>.<br>
(<a href="https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L46" target="_blank">https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L46</a>)<br>
<br>
That makes sense in the HTTP meaning of GET.<br>
<br>
It means, that if we want to get a single instance of a resource (i.e. Task#100) we have to create a new HttpRestProvider with a new URL.<br></blockquote><div><br></div><div>Make sense. Addind get(id)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For delete(), OTOH we can execute it with a specific id, and the same goes for put() to perform an update of a child resource.<br>
<br>
So we have GET, and POST operations performed on a 'parent' resource, and DELETE, and PUT operations performed on child resources. This asymmetry is confusing to me ... non-intuitive.<br></blockquote><div><br></div>
<div>Me too, I did it just because we do not have a way to find the id dynamically yet</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I wouldn't eat exceptions, and return null even in a not-yet-real exception handling :)<br>
(<a href="https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L86" target="_blank">https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L86</a>)<br>
<br>
Really, either don't catch it, or if you do catch it, rethrow it as is, or wrap into another - normally RuntimeException is perfectly fine, so you don't pollute your API with throws declarations.<br></blockquote>
<div><br></div><div>I vote to throw RuntimeException</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I would use IllegalArgumentException here:<br>
<a href="https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/AdapterFactory.java#L34" target="_blank">https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/AdapterFactory.java#L34</a><br>
<br>
It's more appropriate, as it's a standard pattern for this kind of use-case. By convention UnsupportedOperationException is used for empty methods where interface contract is not fully supported.<br></blockquote>
<div><br></div><div>Ok!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- We have to do something about this "getId"<br>
(<a href="https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L76" target="_blank">https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L76</a>)<br>
<br>
One idea is to have an annotation - @Id. But scanning for annotations needs to be done at init time or lazily on first use ...<br>
So maybe we could have an abstract base class with abstract method getId() that every data object has to extend, and implement. A simpler, and more robust solution actually, as compiler will enforce it so there is no way for not providing one, as could happen with forgotten or wrong placement of @Id annotation for example.<br>
</blockquote><div><br></div><div>About the use of annotation I'm already think about. See this todo :) <a href="https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L81" target="_blank">https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L81</a></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
That's about it for now ... :)<br>
<div><br>
- marko<br>
<br>
----- Original Message -----<br>
><br>
><br>
</div><div><div>> Hi guys<br>
><br>
><br>
> I did some changes in the android library based on iOS stuff, it's<br>
> closer to the pipeline adapter implementation. I would appreciate<br>
> feedback and review.<br>
><br>
><br>
> <a href="https://github.com/aerogear/aerogear-android/pull/1" target="_blank">https://github.com/aerogear/aerogear-android/pull/1</a><br>
> <a href="https://github.com/aerogear/aerogear-android-todo/pull/1" target="_blank">https://github.com/aerogear/aerogear-android-todo/pull/1</a><br>
><br>
><br>
> Thanks,<br>
> Passos<br>
</div></div>> _______________________________________________<br>
<div>> aerogear-dev mailing list<br>
> <a href="mailto:aerogear-dev@lists.jboss.org" target="_blank">aerogear-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/aerogear-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/aerogear-dev</a><br>
><br>
</div>_______________________________________________<br>
<div><div>aerogear-dev mailing list<br>
<a href="mailto:aerogear-dev@lists.jboss.org" target="_blank">aerogear-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/aerogear-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/aerogear-dev</a><br>
</div></div></blockquote></div><br>