[aerogear-dev] New Android Pipeline

Daniel Passos daniel at passos.me
Wed Oct 3 08:06:01 EDT 2012


2012/10/2 Marko Strukelj <mstrukel at redhat.com>

>
> 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 don't know. I need to think about this.


> - 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/main/java/org/aerogear/android/core/HttpRestProvider.java#L46
> )
>
> That makes sense in the HTTP meaning of GET.
>
> 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.
>

Make sense. Addind get(id)


> 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.
>
> 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.
>

Me too, I did it just because we do not have a way to find the id
dynamically yet


> - 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/main/java/org/aerogear/android/core/HttpRestProvider.java#L86
> )
>
> 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.
>

I vote to throw RuntimeException


> - I would use IllegalArgumentException here:
>
> https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/AdapterFactory.java#L34
>
> 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.
>

Ok!


> - We have to do something about this "getId"
> (
> https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L76
> )
>
> 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.
>

About the use of annotation I'm already think about. See this todo :)
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L81


> 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 at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/aerogear-dev/attachments/20121003/2cf9043a/attachment-0001.html 


More information about the aerogear-dev mailing list