[PEPr] Comment on Networking::AsteriskManager

Philippe Jausions (http://pear.php.net/user/jausions) has commented on the proposal for Networking::AsteriskManager. Comment: 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? Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=543 -- Sent by PEPr, the automatic proposal system at http://pear.php.net