Philippe Jausions [2008-03-21 23:43 UTC] Since you asked, here are some comments ;-)
- The class itself should be names Net_AsteriskManager, or if there are other sets of APIs beside the manager? Net_Asterisk_Manager
- __construct() cannot return false/true/whatever. Throw an exception on failures.
- Any reason to have the $password member as public, somehow doesn't seem right to me.
- Declare each method with public/protected/private
- Are there any consequences to closing the socket without logging out of the manager first? (and/or what are the advantages to having separate logout() and close())
- login(), logout(), close(), command(), originateCall() and probably other methods should throw an exception on failure
- There is no open() method to re-open the connection after calling a close()
- You probably want to trim the lead whitespaces when sending the multiline commands. i.e. fputs($this->_socket, "blah\r\n"
."blahagain\r\n"); <== note the ."
- Instead of if ($this->_socket) {....} else { return false} do if (!$this->_socket) { return false; }... this way the code is less indented and there's less chasing the end and beginning of if / else blocks.
- What does queues() return? a single value, a delimited-string? An array would seem more adequate. A Net_Asterisk_Queue class might be helpful for the better variable typing too. Same for sipPeers() and iaxPeers()
- Why some method names follow the API (i.e. queueAdd() = Action QueueAdd) when others don't (i.e. logout() = Action Logoff)
- I would lean towards prefixing some of the methods with get, i.e. getQueues() instead of simply queues(). That being said, being close to the original API names has its advantages.
-If possible, try to get more information out of the response when something fails. i.e. What does Action Monitor respond when it fails, beside not having "Success" in it?
Philippe Jausions [2008-03-22 00:07 UTC] And the name of the PHP file should match (part of) the class name, in your case AsteriskManager.php instead of asterisk.php (case sensitive too)
David Jean Louis [2008-03-22 08:33 UTC] I second Philippe's comments, also:
* It seems weird to me to connect to the server in the constructor, I would use a connect() method or use some kind of lazy connection system so the class can be instanciated without connecting immediately to the server;
* I would improve error checking and use exceptions instead of just returning "false" in your method, so you can provide meaningful error messages;
* I would modifiy the __construct() signature to receive a $params array (if you add params later you will not have to modify the signature and break the BC) and also move $username, $password in the login method (there's no need to store them in the class), example:
try {
$asterix = new Net_AsterixManager(array(
'server' => 'example.com',
'port' => 5038
));
$asterix->login('joe', 'secret');
$asterix->command('somecommand');
// ...
} catch (Exception $exc) {
// do something...
}
* I don't know asterix but you should do some validity checks in your command() method (ie: store all authorized commands in a an array and throw an exception if a command is not valid);
DA [2008-03-22 16:14 UTC] Thanks for all your suggestions. The class has now been heavily updated taking into acocunt what you've said. Changelog should give further details.
With regards to two points:
command method - It was suggested to have an array of acceptable commands. This isn't really practical as Asterisk can have numerous plugins installed changing the command set. There can sometimes be hundreds of possible CLI commands. What I have therefore done is to check the response from the server and if it returns the standard command not found error then an exception is thrown.
Queues, IaxPeers, SipPeers return vlaues - The return is a string listing directly from the server. I've looked into providing this as an array but the response from Asterisk seems very variable between Asterisk versions, plugins installed, OS, etc. This is inherent to the API. I could lock it down to specific server setups but I feel this would exclude too many server configurations. It's certainly something I'm still looking into though and if I feel there's a good clean way of returning PHP friendly variables I will do.
David Jean Louis [2008-03-22 18:43 UTC] That's better !
some notes:
* you should definitively check keys of you constructor $params, and make it optional, something like:
function __construct($params = array()) {
if (isset($params['key'])) {
$this->key = $params['key'];
}
// ditto for other params...
}
because someone could do:
$asterisk = new Net_AsteriskManager();
$asterisk->server = 'example.com';
* I found some typos in various methods: $reponse vs $response. It would be cool to start writing some simple unit tests btw ;)
And to be picky:
* There are still un-needed if / else,
* Your file level comment for the licence is weird (3 *** and a line feed),
* maybe an 'autoconnect' key in the $params array would be handy to have, if set to true the constructor calls the connect() stuff
Till Klampaeckel [2008-03-22 18:50 UTC] Looking pretty good. I also like your idea about abstracting the Asterisk API to make it easier for developers to hook their apps to it.
Speaking of returning arrays - they are so 90's! Since Christian (cweiske) suggested it on my last proposal, I kinda fell in love (;-)) with the idea to return objects from a class. An object of a distinct type with methods to ask for data contained in it.
Not sure if I make sense, maybe this link illustrates what I mean:
<http://code.google.com/p/services-projecthoneypot/source/browse/trunk/Services/ProjectHoneyPot/Response/Result.php>
Last but not least - comments on your code:
* you should use visibility (e.g. __construct should be declared public)
* you should have your own exception class (Net_AsteriskManager_Exception which extends PEAR_Exception)
* I'd put your class variables in a private array and implement __set() and __get() so people can change them easily on runtime. (Make sure to document with @property)
And even laster, but not leaster, I am wondering if it makes sense to subclass the feature-set.
E.g. have all monitor related features in their own class, etc.. As you noted different versions and plugins add features, so maybe your class could reflect that. So if I wanted to add support for a plugin myself, I'd subclass Net_AsteriskManager_Plugin_Foobar and that would extend an interface etc..
Just food for thoughts!
DA [2008-03-24 13:34 UTC] Thanks again! All great feedback.
I've done most of the changes you've both mentioned except the following:
Returning objectified data:
I do plan on returning more programmatically defined data than simply text reponses form the Asterisk server. Unfortunately I've come across several problems with servers not returning standard output. This is inherent to Asterisk. This kind of relates to....
Sub-classing features:
This is certainly something I'm going to look into. Whether I'll be able to build all of the sub-classes myself or simply put in the framework and let others do the sub-classes, I'm not sure. Although Asterisk can have many different combinations of features the Manager API command sremain similar. Its the Command method that is the most variable so it may be worth only sub-classing this. Or finding another way of dealing with it.
Basically the developers released the Manager API with an array of commands but then decided to just allow any command they'd not specifically programmed in to be executed via the Command message. So how to treat this is something I'm going to look into.
Separate Exception class:
I'm always wary of creating separate exception classes for such a small class. I'll look into implementing if I expand and subclass the main features (or just the Command method). Is there a clear indicator of when you should create an exception class for your class? At a certain size, age, etc? Or should it just be done as a matter of course?
Thanks again for the feedback - it's much appreciated.
DA [2008-04-19 23:42 UTC] Corrected a bug in the "command" function. Details are available at the projects Google Code page.
I am in the process of completing the documentation for this project and will go to the next status soon.
If anyone has any comments regarding my documentation please don't hesitate to let me know.
Many thanks!
|
