11 Dec
The Story and Rebirth Of Zend_Service_Audioscrobbler, Part 1
Many moons ago I set out (along with my friend Derek) to make a contribution to Zend Framework in the form of an add-on to let people easily access the Audioscrobbler web service. We worked really hard, implemented all the features of the web service at that time. Imagine our surprise when it got accepted as part of the 1.0 release of Zend Framework! Awesome! It even has it's own entry in the manual and everything.
However, I have a confession to make, although it will not come as a surprise to anyone who reads my blog on a regular basis: the code is a complete piece of shit. There. I said it. How do I know that it's terrible and needs refactoring in a fierce way? Check out this lovely snippet of code:
-
//////////////////////////////////////////////////////////
-
/////////////////////// USER ///////////////////////////
-
//////////////////////////////////////////////////////////
-
-
/**
-
* Utility function to get Audioscrobbler profile information (eg: Name, Gender)
-
* @return array containing information
-
*/
-
public function userGetProfileInformation()
-
{
-
$service = "/{$this->get('version')}/user/{$this->get('user')}/profile.xml";
-
return $this->getInfo($service);
-
}
-
-
/**
-
* Utility function get this user's 50 most played artists
-
* @return array containing info
-
*/
-
public function userGetTopArtists()
-
{
-
$service = "/{$this->get('version')}/user/{$this->get('user')}/topartists.xml";
-
return $this->getInfo($service);
-
}
-
-
/**
-
* Utility function to get this user's 50 most played albums
-
* @return SimpleXML object containing result set
-
*/
-
public function userGetTopAlbums()
-
{
-
$service = "/{$this->get('version')}/user/{$this->get('user')}/topalbums.xml";
-
return $this->getInfo($service);
-
}
It goes on and on and freakin' on like this. SEVENTEEN methods just for dealing with user stuff. This is insane. Going back over the comments when I was building this thing I realized I totally ignored created elegant code and instead just 'banged out something that worked'. So, I'm going back to the drawing board and are going to refactor this puppy so it makes sense. First up, let's talk about dealing with users. Wouldn't it be better if we had something like this:
-
public function user($action) {
-
$service = "/{$this->get('version')}/user/{$this->get('user')}/{$action}.xml";
-
return $this->getInfo($service);
-
}
Now, THAT looks like nice and elegant. All I have to do is establish the convention on how to connect to various user-related web services. That's as simple as comments in the file itself. Now, to replicate what I was doing before, here are how simple the calls could be:
-
$zsa = new Zend_Service_Audioscrobbler();
-
$zsa->set('user', 'chartjes');
-
$zsa->set('version', '1.0');
-
$userProfile = $zsa->user('profile');
-
$userTopArtists = $zsa->user('topartists');
-
$userTopAlbums = $zsa->user('topalbums');
I think I just got rid of something like 200 lines of code...and that's just in the user section. Clearly, I majorly screwed it up when I did it the first. Luckily for me, there is built-in testing for all this stuff so I can refactor and test as I go.
Article Tags >> Audioscrobbler || PHP || refactoring || Zend Framework || Zend_Service_Audioscrobbler
Posted by Wil Sinclair on 11.12.07 at 5:23 pm
I'm not sure if you're aware of this, but Zend Framework 1.5 is scheduled for Q1 2008. There's still time to get this refactoring in! In any case, thanks for the contribution.
Posted by Daniel Hofstetter on 11.12.07 at 5:23 pm
Nice refactoring, only the method name "user" is not very descriptive. Maybe something like "getXMLFile" would be better.
Posted by Matt Curry on 11.12.07 at 5:23 pm
Hey Chris, I had no idea you co-wrote the Audioscobbler service for Zend. I used it for my first (and only) Zend app. I had a few issues with it, but it overall it was pretty easy.
Good luck with the refactor.
Posted by Matthew Ratzloff on 11.12.07 at 5:23 pm
Hi Chris,
I can tell you that your proposed solution is not the way to go with your refactoring. The problem with your original design is the fact that it's coupled so closely to Audioscrobbler's API. The solution that you give here would further complicate the API and kill autocompletion for IDEs.
If you haven't already, post about this to the mailing list, or even submit it as a proposal. I'll try to give more detailed feedback than the first round. Darby was on the right track with getChart(), though.
Posted by Chris Hartjes on 11.12.07 at 5:23 pm
@Matthew Ratzloff
My goal here is to reduce the amount of code needed to implement Z_S_A and talk to Audioscrobbler, nothing more. But I find your other comments interesting.
Firstly, I don't see what I'm doing here as 'further complicating the API'. I'm trying to cut down on the ridiculous amount of code I used in the original implementation. The current set of methods for Z_S_A is not about to go away, I'm just going to use the magic of __call() to filter those requests to a much smaller subset of code, nothing more. It's not about changing the API, it's about reducing the amount of code needed to get the job done.
As for killing autocompletion for IDE's, that's not something I ever worry about.
Once I have something more concrete I will post to the Zend Framework mailing list. I think all that will really be argued about is the names of the methods, nothing more.