[PEPr] Comment on Web Services::WindowsDelegatedAuthentication

Till Klampaeckel (http://pear.php.net/user/till) has commented on the proposal for Web Services::WindowsDelegatedAuthentication. Comment: Hey there, (sorry for not commenting earlier) A few quick comments (in no particular order): * please rename to somethinglike Services_WindowsLive_DelegatedAuthentication * please split your classes into one class per file * please use error codes in exceptions (I like to "adhere" to HTTP status codes, those are the most obvious - e.g. 401 for forbidden (wrong credentials), etc. - be creative), use class constants for those errors, this allows anyone to do something like: ... } catch (Services_WindowsLive_Exception $e) { if ($e->getCode() != Services_WindowsLive::ERR_FORBIDDEN) { throw $e; } die('not authorized'); } * if you need multiple exception classes, create a base exception, so people can catch all exceptions from your package easily (for example, class Services_WindowsLive_Exception extends PEAR_Exception, then extend Services_WindowsLive_Exception) * if you haven't done it already, please run PHP_CodeSniffer before you call for votes * simplyfy your code - e.g. instead of: if ($delegationToken) { $this->_delegationToken = $delegationToken; } else { throw new Services_WindowsDelegatedAuthentication_Token( 'delegation token should not be empty' ); } do: if (!$delegationToken) { throw new Services_WindowsDelegatedAuthentication_Token( 'delegation token should not be empty' ); } $this->_delegationToken = $delegationToken; (In general, exit early, avoid too many if/else structures.) * please document your private's (variables) * think about protected vs. private (if I extended your class, private is not as smooth to work with as protected) * please document all your magic calls accordingly (__get, __call, etc.) with @property and @method Anyway, bottom line, good proposal, thanks for working on this and thanks for making the code open source. :) Till Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=563 -- Sent by PEPr, the automatic proposal system at http://pear.php.net