Java Wrapper

[Brandon Gresham]

James Turk suggested I look at writing a Java wrapper around the existing API.

I've looked into your guys' REST API and have to say, it looks top-notch and your dox are superb!

:)

So I figure the wrapper would just facilitate access to the REST API, making the Java developer almost oblivious that aspect of how they get the data and offering them instead a Java interface to call methods such as:

billSearch( ... ) : Bill

I also figure this sort of project needs decent unit-testing.  So I opened up the Python code and took a look around... now, I'm not a Python guy (and am constantly distracted by my 2-year-old!), so I could very well have missed it -- but I don't see anything that looks like a set of unit-tests... however, I do see what appear to be test-data (/openstates/manual_data).  So I could recycle that data into my tests, and just write my own tests totally from scratch -- but if there are existing python unit-tests for me to model my tests after, that would be great to have.

Having said all of that, I would very much appreciate any feedback anyone has for me before I start inching forward on this.  I'm a Java guy, but I've never really written a public API before and my current shop (and to some extent, my previous one too) mostly has the motto "working code wins; pretty code be damned".  Of course, I try to use modern patterns and all that but without any critical review going on, well... 

IOW: I want to write to YOUR community's standards; I don't want to produce anything anyone would be embarrassed about later on.  :)

So, python guys or not, I invite any feedback/code-review you feel like offering.

:)

 

[Paul R. Tagliamonte]

On Sat, Mar 17, 2012 at 10:53 AM, Brandon Gresham
<bra...@thegreshams.net> wrote:
> James Turk suggested I look at writing a Java wrapper around the existing
> API.
>
> I've looked into your guys' REST API and have to say, it looks top-notch and
> your dox are superb!
> :)
>
> So I figure the wrapper would just facilitate access to the REST API, making
> the Java developer almost oblivious that aspect of how they get the data and
> offering them instead a Java interface to call methods such as:
>
> billSearch( ... ) : Bill
>
>
> I also figure this sort of project needs decent unit-testing.  So I opened
> up the Python code and took a look around... now, I'm not a Python guy (and
> am constantly distracted by my 2-year-old!), so I could very well have
> missed it -- but I don't see anything that looks like a set of unit-tests...
> however, I do see what appear to be test-data (/openstates/manual_data).  So
> I could recycle that data into my tests, and just write my own tests totally
> from scratch -- but if there are existing python unit-tests for me to model
> my tests after, that would be great to have.

Unit testing API bindings are very hard. By the way, the API
implementation for OpenStates is in python-sunlight[1], these bindings
are for developers who wish to use the API (e.g. provide a jar that
you can invoke to chat with the OpenStates server), not the server API
implementation it's self :)

>
> Having said all of that, I would very much appreciate any feedback anyone
> has for me before I start inching forward on this.  I'm a Java guy, but I've
> never really written a public API before and my current shop (and to some
> extent, my previous one too) mostly has the motto "working code wins; pretty
> code be damned".  Of course, I try to use modern patterns and all that but
> without any critical review going on, well...

I'd be happy to help as much as I can :)

>
> IOW: I want to write to YOUR community's standards; I don't want to produce
> anything anyone would be embarrassed about later on.  :)
>
> So, python guys or not, I invite any feedback/code-review you feel like
> offering.
> :)

Sounds awesome. Where do you think you'll be hosting this? I'd love to
watch it develop :)

>
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Open State Project" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/fifty-state-project/-/DDiQ4uwPx1cJ.
> To post to this group, send email to fifty-sta...@googlegroups.com.
> To unsubscribe from this group, send email to
> fifty-state-pro...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/fifty-state-project?hl=en.

[1]: https://github.com/sunlightlabs/python-sunlight

Cheers,
Paul

--
I'm Paul.

[Brandon Gresham]

On Mar 17, 2012, at 1:25 PM, Paul R. Tagliamonte wrote:
> Unit testing API bindings are very hard.

I wasn't necessarily planning on testing the API-bindings per-se -- my plan was to inject test-data into my interface and make sure it creates the expected model as output. That way, as things evolve, I can be sure I'm maintaining backwards-compatibility and all that. Whether or not the actual API changes is another matter -- and you're right, I can see how it would be hard to test that.

Or, were you getting at something different?

> By the way, the API
> implementation for OpenStates is in python-sunlight[1], these bindings
> are for developers who wish to use the API (e.g. provide a jar that
> you can invoke to chat with the OpenStates server), not the server API
> implementation it's self :)

I'm not sure I'm following… are you suggesting I not use the REST API to accomplish the goal?

> I'd be happy to help as much as I can :)
>

I'm counting on it!
:)

>
> Sounds awesome. Where do you think you'll be hosting this? I'd love to
> watch it develop :)
>

I figured to just keep it here on GitHub. Suggestions, different idea?

[Brandon Gresham]

Quick "?".... would it be best to only include the standard fields or to provide state-specific models so as to include the optional fields?

[Eric Mill]

You don't have to choose, I think. You can provide one model that works for every state and has every field. Two possible approaches:

* Have a property defined for every optional field that you know about, which users of your API client should expect to possibly be null and to plan accordingly, or

* Have standard API fields in defined properties, and then toss every +-prefixed field into a HashMap<String,Object> for users that know what they're looking for.

The second option has the advantage of automatically capturing new fields that the Open States team adds to the API, without having to update your code to handle any of them. 

The one issue I see (with either option) is handling when the Open States team "graduates" +-prefixed fields to standard fields. If you were to go with the HashMap approach, you may want to also make that hash a catch-all for unrecognized standard fields, and strip off the "+" from everything you put in, so that if "+short_title" becomes "short_title" on the server side, it doesn't affect anyone.

-- Eric

On Sat, Mar 17, 2012 at 9:54 PM, Brandon Gresham <bra...@thegreshams.net> wrote:

Quick "?".... would it be best to only include the standard fields or to provide state-specific models so as to include the optional fields?

--
You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/1OyBCIGrRM4J.

To post to this group, send email to fifty-sta...@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-pro...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Brandon Gresham]

Eric,

Excellent idea -- really like the HashMap-approach, thanks!  

Am toying with the idea of using GSON, liking it so far -- but supporting all non-recognized fields with a HashMap-backing, not sure yet how to do that in GSON; going to look into that now.

Thanks again!

On Saturday, March 17, 2012 10:20:17 PM UTC-6, Eric Mill wrote:

You don't have to choose, I think. You can provide one model that works for every state and has every field. Two possible approaches:

* Have a property defined for every optional field that you know about, which users of your API client should expect to possibly be null and to plan accordingly, or

* Have standard API fields in defined properties, and then toss every +-prefixed field into a HashMap<String,Object> for users that know what they're looking for.

The second option has the advantage of automatically capturing new fields that the Open States team adds to the API, without having to update your code to handle any of them. 

The one issue I see (with either option) is handling when the Open States team "graduates" +-prefixed fields to standard fields. If you were to go with the HashMap approach, you may want to also make that hash a catch-all for unrecognized standard fields, and strip off the "+" from everything you put in, so that if "+short_title" becomes "short_title" on the server side, it doesn't affect anyone.

-- Eric

On Sat, Mar 17, 2012 at 9:54 PM, Brandon Gresham <bra...@thegreshams.net> wrote:

Quick "?".... would it be best to only include the standard fields or to provide state-specific models so as to include the optional fields?

--
You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/1OyBCIGrRM4J.

To post to this group, send email to fifty-state-project@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-project+unsub...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Brandon Gresham]

Eric,

OK, looks like I found a solution to option-2.  

I can use Jackson to auto-bind the standard fields and then declare a deserialization-handler that is triggered anytime Jackson finds a field that isn't declared in my Object -- in the handler I can get the field components and add them to the Object's custom-property map.

Still need to play around with the potential to automatically-handle *NEW* standard properties, but I know I can at least handle optional-properties (ie: "+") dynamically, so that's pretty nice.

On Saturday, March 17, 2012 10:20:17 PM UTC-6, Eric Mill wrote:

You don't have to choose, I think. You can provide one model that works for every state and has every field. Two possible approaches:

* Have a property defined for every optional field that you know about, which users of your API client should expect to possibly be null and to plan accordingly, or

* Have standard API fields in defined properties, and then toss every +-prefixed field into a HashMap<String,Object> for users that know what they're looking for.

The second option has the advantage of automatically capturing new fields that the Open States team adds to the API, without having to update your code to handle any of them. 

The one issue I see (with either option) is handling when the Open States team "graduates" +-prefixed fields to standard fields. If you were to go with the HashMap approach, you may want to also make that hash a catch-all for unrecognized standard fields, and strip off the "+" from everything you put in, so that if "+short_title" becomes "short_title" on the server side, it doesn't affect anyone.

-- Eric

On Sat, Mar 17, 2012 at 9:54 PM, Brandon Gresham <bra...@thegreshams.net> wrote:

Quick "?".... would it be best to only include the standard fields or to provide state-specific models so as to include the optional fields?

--
You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/1OyBCIGrRM4J.

To post to this group, send email to fifty-state-project@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-project+unsub...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Brandon Gresham]

OK, figured it out; whether or not it's a discrete-value, I can stuff it in the map even if it's not prefixed with a "+".  But, if it's not a discrete-value, it will be a JSON-string; figure, that's better than nothing and the end-user can parse it if they want it.  I guess I could iteratively parse over it and build additional nest map(s) that get shoved into my map... that way there's not a ton of JSON for the end-user to have to deal with.  

Maybe I'll circle back on that at a later point, though.

[Eric Mill]

That sounds good. As long as you can strip off the + sign before you store it in the hash, you'll handle the graduation of experimental to standard fields automatically.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/aMqxETORFtUJ.
To post to this group, send email to fifty-sta...@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-pro...@googlegroups.com.

[Brandon Gresham]

Already stripping off the + sign.  ;)

Really appreciate the feedback on this; much nicer than the alternative I was considering beforehand.

On Sunday, March 18, 2012 4:06:01 PM UTC-6, Eric Mill wrote:

That sounds good. As long as you can strip off the + sign before you store it in the hash, you'll handle the graduation of experimental to standard fields automatically.

On Mar 18, 2012 12:36 PM, "Brandon Gresham" <bra...@thegreshams.net> wrote:

OK, figured it out; whether or not it's a discrete-value, I can stuff it in the map even if it's not prefixed with a "+".  But, if it's not a discrete-value, it will be a JSON-string; figure, that's better than nothing and the end-user can parse it if they want it.  I guess I could iteratively parse over it and build additional nest map(s) that get shoved into my map... that way there's not a ton of JSON for the end-user to have to deal with.  

Maybe I'll circle back on that at a later point, though.

On Sunday, March 18, 2012 8:58:34 AM UTC-6, Brandon Gresham wrote:

Eric,

OK, looks like I found a solution to option-2.  

I can use Jackson to auto-bind the standard fields and then declare a deserialization-handler that is triggered anytime Jackson finds a field that isn't declared in my Object -- in the handler I can get the field components and add them to the Object's custom-property map.

Still need to play around with the potential to automatically-handle *NEW* standard properties, but I know I can at least handle optional-properties (ie: "+") dynamically, so that's pretty nice.

On Saturday, March 17, 2012 10:20:17 PM UTC-6, Eric Mill wrote:

You don't have to choose, I think. You can provide one model that works for every state and has every field. Two possible approaches:

* Have a property defined for every optional field that you know about, which users of your API client should expect to possibly be null and to plan accordingly, or

* Have standard API fields in defined properties, and then toss every +-prefixed field into a HashMap<String,Object> for users that know what they're looking for.

The second option has the advantage of automatically capturing new fields that the Open States team adds to the API, without having to update your code to handle any of them. 

The one issue I see (with either option) is handling when the Open States team "graduates" +-prefixed fields to standard fields. If you were to go with the HashMap approach, you may want to also make that hash a catch-all for unrecognized standard fields, and strip off the "+" from everything you put in, so that if "+short_title" becomes "short_title" on the server side, it doesn't affect anyone.

-- Eric

On Sat, Mar 17, 2012 at 9:54 PM, Brandon Gresham <bra...@thegreshams.net> wrote:

Quick "?".... would it be best to only include the standard fields or to provide state-specific models so as to include the optional fields?

--
You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/1OyBCIGrRM4J.

To post to this group, send email to fifty-state-project@googlegroups.com.

To unsubscribe from this group, send email to fifty-state-project+unsubscribe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Brandon Gresham]

OK, project is published here: https://github.com/bane73/openstates4j

It currently only includes the model Bill (and sub-models).  It's a little rough still, but pretty well complete for what it is.  A static runner is included just as a simple test.  If you want to run it, be sure to include a Program Argument (in your Run Configuration): apiKey=YOUR_OPEN_STATES_API_KEY

I would really appreciate it if someone would review it and holler-up if you see anything particularly ugly/worrisome.

Note: the Bill class was written roughly in the Builder-pattern whereas the other sub-models were not.  I like the Builder-pattern just so we know the objects are immutable, but in reality they are anyways (mostly) since I'm not providing any mutators (I think the only one deals with the optional-properties).  Thoughts???  Should I expand the others to follow the Builder-pattern, drop it as unnecessary, or do something different?

[Eric Mill]

You'll have to forgive me, I never deal with Java besides in an Android context. What's the best way for me to run and test this code?

--

You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/nTHwyx6Bij4J.

To post to this group, send email to fifty-sta...@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-pro...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Paul R. Tagliamonte]

On Tue, Mar 20, 2012 at 12:22 AM, Brandon Gresham
<bra...@thegreshams.net> wrote:

> OK, project is published here: https://github.com/bane73/openstates4j
>
> It currently only includes the model Bill (and sub-models).  It's a little
> rough still, but pretty well complete for what it is.  A static runner is
> included just as a simple test.  If you want to run it, be sure to include a
> Program Argument (in your Run Configuration):
> apiKey=YOUR_OPEN_STATES_API_KEY
>
> I would really appreciate it if someone would review it and holler-up if you
> see anything particularly ugly/worrisome.

Hurm. Well, It's really clear you spent a great deal of time getting
this together, so before I start anything, thank you for your work!

One bit that I would have done differently would be some of the
mechanics of the library - IMHO it's up to the API wrapper to access
the server's URL, decode the response and populate a POJO, if we are
going the POJO route (which it looks like you are)

The way I wrote the python-sunlight binding is actually slightly
different, but I think it's approach might actually work here too -

There's an OpenStates object (instantiated as openstates) with a
couple of methods (openstates.bills(), openstates.legislators() ...),
which take a set of arguments to describe the fetch (e.g. -
openstates.bills(state="co"))

I know doing optional named params or overloading isn't fun, so
perhaps a HashMap<String> could work in it's place, or something.

This method in python-sunlight just returns a data structure as
decoded by the json library directly through, which reduces a lot of
complexity, and makes it easy for other people to use the data.
Sometimes too much abstraction is a bad thing(tm).

Returning the decoded data from the server might be better then
populating POJOs, since you'd have to create instance variables and
getters/setters for all the elements that *could* come back from *any*
state - or use a HashMap (or something) and compose that.

Simple is better ;)

>
> Note: the Bill class was written roughly in the Builder-pattern whereas the
> other sub-models were not.  I like the Builder-pattern just so we know the
> objects are immutable, but in reality they are anyways (mostly) since I'm
> not providing any mutators (I think the only one deals with the
> optional-properties).  Thoughts???  Should I expand the others to follow the
> Builder-pattern, drop it as unnecessary, or do something different?

Is there any reason you needed this pattern? I would have thought
(guessing from not reading the code) you you'd built a concrete
builder for each state (or something), and populated the POJOs (e.g.
OhioConcreteBillBuilder, etc) depending on the state, but I see it's
just one, so I was wondering why you choose to go that way.

I'd also consider adding the fetch mechanics into some top level
OpenStates object or something, and returning either a decoded
response, or some sort of array of Bill objects or something.

I could be missing something, it's been a while since I've done Java

>
> --
> You received this message because you are subscribed to the Google Groups
> "Open State Project" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/fifty-state-project/-/nTHwyx6Bij4J.
>
> To post to this group, send email to fifty-sta...@googlegroups.com.
> To unsubscribe from this group, send email to
> fifty-state-pro...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/fifty-state-project?hl=en.

Thanks again for your hard work!

[Eric Mill]

I guess I'll add in my thoughts here too, then -

The Builder pattern seems a bit superfluous to me. It's no big deal if the object is mutable or not - as Van Rossum says, we're all consenting adults here. But more importantly, having to initialize the Bill class and run the fetch operation by using a Builder object is a lot more complicated than calling Bill.find(state, session, chamber, bill_id), or Bill.search(query) or the like.

I feel similarly about getters and setters over just making instance variables public, though I know that's pretty universally practiced in the Java community. Still, just ditching those cuts down on the LOC by like 3-fold.

It seems to me like you could also cut down on the footprint of the client, and still get all the value from it, by taking the models that only appear in the context of a Bill (like Action, Vote, Document, Version, Source) and making them inner classes of Bill (e.g. Bill.Action).

-- Eric

--

Developer | sunlightfoundation.com

[Brandon Gresham]

Wow, great feedback!  Thank you so much to both of you.

I'm at work at the moment so I don't want to take a ton of time replying right now; will try and get back to your questions later in the day.

Let me just start by saying that this code is nowhere near complete yet... I have not yet provided any top-level service-layer or great openstates-object.  This was just an attempt to show my Bill-pojo to get feedback whether I was going the approach the OS-community would find friendly or not.

Clearly I am not.  ;)

And I'm totally fine with that -- it's the reason I asked in the first-place!  :D

I really appreciate the feedback -- and will circle-back to answer your questions later on today, and probably greatly revise my approach.

[Eric Mill]

In case I didn't make it clear, I think this is totally going in the right direction. I'm super glad you're writing it, and are working hard to figure out the best approach. It's going to be very useful when it's done.

--
You received this message because you are subscribed to the Google Groups "Open State Project" group.

To view this discussion on the web visit https://groups.google.com/d/msg/fifty-state-project/-/uCESl9e3cIUJ.

To post to this group, send email to fifty-sta...@googlegroups.com.
To unsubscribe from this group, send email to fifty-state-pro...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/fifty-state-project?hl=en.

--

Developer | sunlightfoundation.com

[Paul R. Tagliamonte]

On Tue, Mar 20, 2012 at 11:33 AM, Eric Mill <er...@sunlightfoundation.com> wrote:
> In case I didn't make it clear, I think this is totally going in the right
> direction. I'm super glad you're writing it, and are working hard to figure
> out the best approach. It's going to be very useful when it's done.

Seconded (sorry for the ML spam, I just want to make sure I was as
clear as Eric here)

--
I'm Paul.

[Brandon Gresham]

Finally got some free-time.

I want to start by thanking you both again for your input; I really value it.  The fact that you are both developers with a different background (ie: python) and experience in the open-source world, I am trying to learn from that.  :)

Here's my responses to your questions/thoughts:

>> "IMHO it's up to the API wrapper to access the server's URL, decode the response and populate a POJO" (PAUL)

I think what you are wondering about here is where the service-layer is?  I haven't gotten to that part yet.  Just wanted to show the direction I was headed with my model before I got too far down a rat-hole.  

>> "This method in python-sunlight just returns a data structure as decoded by the json library directly through, which reduces a lot of complexity, and makes it easy for other people to use the data.

Sometimes too much abstraction is a bad thing(tm). Returning the decoded data from the server might be better then populating POJOs" (PAUL)

I think what you are saying is to just de-jsonify the data and stuff it into a simple key/value-map.  Let the user just have simpler access to the data but don't worry about massaging/object-ifying it too much.  I'm guessing something really simple, sort of like this:

public Bill {

public Map<String, String> data;

}

This is a tempting approach.  I have always bent towards the more object-ified approach, but making it this simple sure is nice in terms of complexity on my end.

>> "Is there any reason you needed this pattern?" (PAUL)

No, and I thought against it at the time -- which is why I didn't do it in the other objects yet.  Thought I'd get an opinion or two from the community on the matter.  I'm a fan of Bloch's book, where he espouses this pattern a fair amount, but it's possible this is overkill and inappropriate for this situation.  I like things to be immutable, but there often isn't an overwhelming need for them to be so.  Especially since this project consists only of data one can consume; if YOU change it for YOUR app, who really cares?  It's not like your app can accidentally change the data and inject it back into the API.

>> "I would have thought(guessing from not reading the code) you you'd built a concrete builder for each state (or something), and populated the POJOs (e.g. OhioConcreteBillBuilder, etc) depending on the state, but I see it's just one, so I was wondering why you choose to go that way." (PAUL)

Yeah, that was a "?" I thought about at the very start; Eric gave me some direction on that that I really liked, saved me a ton of state-specific coding, which I really liked.  The properties labeled by the API as 'standard' get their own fields; everything else that comes through (which represents state-specific variability) just gets shoved in a simple key/value-map.

>> "I'd also consider adding the fetch mechanics into some top level OpenStates object or something, and returning either a decoded response, or some sort of array of Bill objects or something." (PAUL)

Intending to.  Just not done yet.

>> "The Builder pattern seems a bit superfluous to me..." (ERIC)

Agreed.  After hearing both of you chime in to this effect, I will remove the Builder pattern.  :)

>> "I feel similarly about getters and setters over just making instance variables public, though I know that's pretty universally practiced in the Java community. Still, just ditching those cuts down on the LOC by like 3-fold." (ERIC)

AH, ANATHEMA!!!!!!  :)

Yeah, you're right... making the props public is A Very Bad Thing in the Java world, no doubt about it.  I would feel to be sinning if I were to do so.  Having said that, I live in Utah, swear like a sailor, and consume large amounts of coffee -- so, perhaps marking them public is in keeping with my nature.  ;P

I will have to ponder that one.  I can def. see the nicety about it from a simplistic perspective.

>> "It seems to me like you could also cut down on the footprint of the client, and still get all the value from it, by taking the models that only appear in the context of a Bill (like Action, Vote, Document, Version, Source) and making them inner classes of Bill (e.g. Bill.Action)." (ERIC)

I was planning on it but then the Bill class itself was already decently large enough (due largely to the Builder-pattern I'm going to nix, and partly due also to the getters) that the class's footprint was just getting too big for my tastes.  Now that I'm killing the Builder, I might like it better to go back to this approach; especially if I decide to get rid of the getters.

Once again, I appreciate this very much.

• Gonna kill the Builder
• Gonna include a service-layer
• Gonna stick with a single Bill for all states, using specific properties for all the standard ones, and a map for all the state-optional properties
• Gonna move the Bill-specific sub-models into the Bill class itself
• Considering making the properties public and removing the getters -- Paul, what's your thoughts on that?

Any additional thoughts?  Did I misunderstand anything?