Hello,
On Wed, Oct 3, 2012 at 12:37 AM, Marko Strukelj <mstrukel(a)redhat.com> wrote:
Some comments ... (I haven't looked at this that closely before so there are some
things here that relate to much earlier commits).
- 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.
- 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 :)
- 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
http://host/task, and not to something like
http://host/task/100.
(
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/ma...)
That makes sense in the HTTP meaning of GET.
I'd use plural for endpoint URIs (e.g. /tasks and not /task)
We need get() for retrieving _all_ items and get(id) for retrieving a
single object (=> /tasks/{id}).
(We also need 'queries', eventually)
Regarding iOS... I left a TODO on the more high level 'filter' read
(not implemented in the rest adapter)
=>
https://github.com/aerogear/aerogear-ios/blob/master/AeroGear-iOS/AeroGea...
The 'filterObject' argument *could* be:
- an id to look up a single entity
- some more complex query object (e.g. a range or what not)
==> this all would translate in to a 'get()' invocation, where the
method takes an argument.
For looking up a single entity, of course I could have added a
'read(id)', which invokes an underlying 'http.get(id)'; Perhaps I add
that read(id) now :)
Anyways, back to Android... get() is fine for all, BUT get(id) is also needed.
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.
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.
Correct, a put on /tasks does make no sense. A delete on /tasks would
(I guess in most cases) mean all items are deleted, which feels
wrong....
So IMO these put/delete operations make more sense on URIs, like
/tasks/{id} (update / remove a single item).
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.
Hrm... I disagree, as indicated in my sentences on put/delete above...
I like Stefan's image on restful URIs:
http://www.infoq.com/resource/articles/rest-introduction/en/resources/fig...
The above picture 'models' URIs for this UML diagram:
http://www.infoq.com/resource/articles/rest-introduction/en/resources/fig...
(Taken from this article =>
http://www.infoq.com/articles/rest-introduction)
However the figure2.jpg shows as well, that there maybe cases where a
DELETE on /tasks (or '/customers/{id}/orders') and a POST on
/tasks/{id} (or '/orders/{id}') can make sense.
=> We need to cover that too...
If I recall correctly those two 'corner cases' are also not covered by
the JavaScript library.
- I wouldn't eat exceptions, and return null even in a
not-yet-real exception handling :)
(
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/ma...)
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.
+1 on this comment - but he added a TODO already ;)
- I would use IllegalArgumentException here:
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/ma...
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.
+1
- We have to do something about this "getId"
(
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/ma...)
One idea is to have an annotation - @Id. But scanning for annotations needs to be done at
init time or lazily on first use ...
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.
I think the problem here is that not every object has an 'id', the
field could be name 'recordId' - In JavaScript this is configurable.
iOS has a TODO here...
-Matthias
That's about it for now ... :)
- marko
----- Original Message -----
>
>
> Hi guys
>
>
> I did some changes in the android library based on iOS stuff, it's
> closer to the pipeline adapter implementation. I would appreciate
> feedback and review.
>
>
>
https://github.com/aerogear/aerogear-android/pull/1
>
https://github.com/aerogear/aerogear-android-todo/pull/1
>
>
> Thanks,
> Passos
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev(a)lists.jboss.org
>
https://lists.jboss.org/mailman/listinfo/aerogear-dev
>
_______________________________________________
aerogear-dev mailing list
aerogear-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/aerogear-dev
--
Matthias Wessendorf
blog:
http://matthiaswessendorf.wordpress.com/
sessions:
http://www.slideshare.net/mwessendorf
twitter:
http://twitter.com/mwessendorf