Skip to content

Conversation

@azonli
Copy link
Contributor

@azonli azonli commented Feb 22, 2013

I've taken the liberty to streamline twitter list access to make it more analogous to the way other resources are accessed in the library:

  • only the json parsing is now done in the TwitterList object, for the rest it's just a data container
  • most methods in the Twitter object related to twitterlists are now moved to a new object Twitter_Lists
  • access to the Twitter_Lists object is done via Twitter.lists() (compare with Twitter_Users access via Twitter.users())
  • (almost) all methods requiring one or more calls to twitter for twitterlists are moved out of the TwitterList
  • I've adjusted the TwitterListTest class to work with the new Twitter_Lists object instead of with the TwitterList object directly
  • methods Twitter.addStandardishParameters(Map<String, String>) and Twitter.getStatuses(String, Map<String, String>,boolean) have been made package private to be able to access them from within Twitter_Lists

N.B. unfortunately the .gitignore has been committed and I'm not that handy with git yet to exclude it from this pull request; it only adds binlocal and .settings to ignore so it's not too bad, I guess

to clean up the clutter in the Twitter object, I've extracted most
twitterlist-releated methods from the Twitter object into a new object
Twitter_Lists which is accessed the same as Twitter_Users, i.e.
Twitter.lists() vs Twitter.users()

also, the TwitterList object no longer makes direct calls to twitter,
again analogous to other objects in the library: only the json parsing
is done in TwitterList, the calls are mostly handled by Twitter_Lists
@winterstein
Copy link
Owner

Hi Azonli,

Thanks for the input -- I really appreciate it.

My quick reaction is I'm not sure about all these changes.

I like your use of Twitter_Lists to standardise with Twitter_Users,
etc, and cleaner factoring of the tests.

I'm less sure about removing the List support from TwitterList. Having
TwitterList as a lazy-loading List provides an easy & familiar
interface, and it can result in less calls to Twitter than a
load-all-the-members approach.

What's your thinking behind that change?

Best regards,

  • Dan

@azonli
Copy link
Contributor Author

azonli commented Feb 22, 2013

Well, the approach I've chosen is that a TwitterList object represents the information returned from twitter when showing/creating/updating a list, not as a list of twitter users. The application I'm using jtwitter for, has a need to keep track of the ratelimits remaining and so needs to know in advance how many calls a particular method might take and that can be done by first accessing the TwitterList's data.

When I looked at the original code, the only real advantage I saw for using the TwitterList as a lazy loading list of members was the ability to get a member at a specific position in a list via the TwitterList.get(int) method. What's the point in lazy-loading the list data if you're sure you're not using the list anyway? Hence the seperation of action methods into the Twitter_Lists object, where you know that you're acting on a list.

Furthermore, I found the real creation of a list by constructor code not nice and found the code as a whole in the TwitterList object a bit messy. My separation of twitter-calls-using methods into Twitter_Lists and twitterlist-data in TwitterList was my way to clean it up :)

I do see a drawback that my approach possibly introduces a lot of extra methods in Twitter_Lists to support the lookup by slug+owner vs just the id of a list.

Thinking about the list support a bit further, I can imagine that using a sublist approach would help when paging through a list of members. I guess what would be handy is adding cursor support to both the Twitter_Lists getListMembers and getListSubscribers methods, bearing in mind that originally paging support was not implemented for subscribers ;-)

Still think it's needed to keep the list support?
Sorry for the long comment, btw...

Regards,
Anton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants