Travis Swicegood [2007-10-23 02:02 UTC] Cool... now I don't have to write one :-)
Just running commentary as I go through it:
* What about moving the connection data out of Services_Facebook_Common into Services_Facebook_Connection, the making the Common class instantiate the connection? That allows for changes to happen specific to the connection without touching every single object (by inheritance). I'm specifically thinking of Services_Facebook_Connection_Driver classes to handle curl, sockets, pecl/http, etc., etc.
* I would move Services_Facebook_Common::checkRequest() into a Services_Facebook_Util class as a public so it can be explicitly tested
* The Users::$userFields array needs to be accessible somehow. I'm thinking of the cases where you want everything but one or two fields, or in future cases where you want everything and a few new fields. Making it public doesn't seem like a great idea though. Possibly making it protected so you could sub-class S_F_Users and add your own default fields. How about this: make $userFields protected, and add a public static $defaultUserFields that is used on each instance to start the $userFields.
* Services_Facebook_Photos::$imageTypes is similar to above. It's not as big of deal here, but just for future extensibility when the hot new image type is supported and you've moved on to Python or Ruby :-)
* What is Services_Facebook::isValidRequest() doing? I don't see a call to it anywhere in the code. Has grep failed me?
The only other thing is more stylistic and has more to do with a personal preference. With that preface out of the way, I would want to instantiate Services_Facebook and use an API along these lines:
<code>
$facebook = new Services_Facebook('my-api-key', 'my-secret');
$facebook->groups->getMembers('gid');
$facebook->photos->getAlbumsByUser('some-uid');
$facebook->users->getLoggedInUser();
$facebook->users->isAppAdded();
</code>
That reads more like what is seen in the API docs. The various properties could be lazy-loaded via __get(), removing the need to explicitly call factory() all together.
To write an API that I would like to use versus the standard Facebook API, I would add classes such as Services_Facebook_User (no 's') that would be my interface to the rest of the world.
<code>
$user = $facebook->getCurrentUser();
// complemented by a $facebook->getUser('uid');
$user->isFriendsWith('facebook-user-id');
$user->getInfo();
$user->groups; // Services_Facebook_Group[]
$user->groups['gid|group name']->members['uid|handle'] instanceof Services_Facebook_User
$user->publishStory('title', 'body');
$user->publishAction('title', 'body');
$user->albums['aid|name']->photos; // Services_Facebook_Photo[]
</code>
That might be something better suited to building on top of a Services_Facebook package as Services_Facebook_Extended. :-)
Till Klampaeckel [2007-10-24 20:45 UTC] I like the idea a lot. Great work, haven't had time to dive into internals, but I will later on. Thanks for the hard work.
Hope you get the API-issues with facebook's help resolved. Looks like they are interested.
Joe Stump [2007-10-28 02:24 UTC] > * What about moving the connection data out of
> Services_Facebook_Common into Services_Facebook_Connection, the
> making the Common class instantiate the connection? That allows for
> changes to happen specific to the connection without touching every
> single object (by inheritance). I'm specifically thinking of
> Services_Facebook_Connection_Driver classes to handle curl, sockets,
> pecl/http, etc., etc.
I'd never implement the driver interface mainly because I doubt a person cares *how* you're connecting to Facebook. That being said, when PEAR2's HTTP_Request comes out I'll use that and add an accept method to you can make connections however you want.
> * I would move Services_Facebook_Common::checkRequest() into a
> Services_Facebook_Util class as a public so it can be explicitly
> tested
When would you need to check results explicitly? That's handled in the request method anyways.
> * The Users::$userFields array needs to be accessible somehow. I'm
> thinking of the cases where you want everything but one or two
> fields, or in future cases where you want everything and a few new
> fields. Making it public doesn't seem like a great idea though.
> Possibly making it protected so you could sub-class S_F_Users and
> add your own default fields. How about this: make $userFields
> protected, and add a public static $defaultUserFields that is used
> on each instance to start the $userFields.
Hrm. Good idea. How about a setDefaultUserFields()? I'm fine even making it public. I now see a reason to expose it; it's just a matter of how to go about exposing it?
> * Services_Facebook_Photos::$imageTypes is similar to above. It's
> not as big of deal here, but just for future extensibility when the
> hot new image type is supported and you've moved on to Python or
> Ruby :-)
The joys of Open Source; when I've bailed for Python you all can add the new image type.
> * What is Services_Facebook::isValidRequest() doing? I don't see a
> call to it anywhere in the code. Has grep failed me?
This function checks *incoming* requests to your server. You'd use it to validate requests to your canvas callback page are, in fact, coming from Facebook. Hence the reason it's not used else where since the other classes are all outgoing.
> <code>
> $facebook = new Services_Facebook('my-api-key', 'my-secret');
> $facebook->groups->getMembers('gid');
> $facebook->photos->getAlbumsByUser('some-uid');
> $facebook->users->getLoggedInUser();
> $facebook->users->isAppAdded();
> </code>
>
> That reads more like what is seen in the API docs. The various
> properties could be lazy-loaded via __get(), removing the need to
> explicitly call factory() all together.
I like this a lot. It makes sense. Would you then make Services_Facebook::factory() private? Or leave it exposed?
> That might be something better suited to building on top of a
> Services_Facebook package as Services_Facebook_Extended. :-)
Yeah, how about version 2.0? :-)
Joe Stump [2008-03-31 02:41 UTC] I've addressed all of Travis's concerns and implemented lazy loading via Services_Facebook::__get(). I've also set up a repository everyone to browse at Google Code (http://code.google.com/p/servicesfacebook). Finally, Jeff Hodsdon and Travis Swicegood have both been suckered into help maintain this beast. I'll be proposing a vote in the coming days. Please poke and prod the new code (the links are broken so check things out on Google Code).
|
