[PEPr] Comment on Authentication::NTLMProxy

Justin Patrin (http://pear.php.net/user/justinpatrin) has commented on the proposal for Authentication::NTLMProxy. Comment: > > sprintf is far slower than any other string creation method. Single > quotes > > are the preferred method. See: > > http://pear.reversefold.com/strings/ > > It is especially inappropriate to use sprintf when you're not even > doing > > anything to the string (such as sprintf("\r\n\r\n")...). > > if speed is the only problem then I'll be happy. ;-) Your call... Using simple concatenation is also IMHO easier to read, but this isn't something that must be done to get accepted. > I remove the unnecessary sprintf(). > > > This package should use HTTP_Request and/or HTTP_Client. If there is a > > problem with keep-alive I doubt it would be too hard to alter > HTTP_Client > > and Request to support it. They could easily just keep the socket open > and > > do more sending/receiving. > > If it's easily, please fix > http://pear.php.net/bugs/bug.php?id=4806 > first. I'll try to take a look, but am very busy. You already have some code to deal with this, would it be all that hard to simply patch HTTP_Request/Client to support this? Even if you don't do this, why not use Net_Socket instead of using low-level socket manipulation functions? > > die()/exit should not be called in a package. Why can't the script > simply > > exit normally? > > The need of die() is that there's a rea big problem when this situation > happened... You need to explain this in more detail. The script should be able to exit on its own, shoudl it not? If you write your code to return instead of die then the calling script can end and nothing untoward shoudl happen. > The exit() calls are needed during the NTLM handshake. You keep saying this but never explain why. Why should this be needed? Why won't letting the script end normally work? > > > _createNewSocket and _createSocket duplicate code. > > They don't duplicate code, _createNewSocket() closes an open socket > first. Yes they do. All of the code is the same except the close call. Have _createNewSocket call _createSocket instead of duplicating the code. > > The log is a decent I idea, but I suggest you use the Log package > instead > > as it would allow the developer to log however they want to. > > I do not want a bloated class. Sorry, that's not a good reason, especially since supporting Log would add only a few lines of code. Your current log functionality only appends to a string. Allowing someone to use Log would be far more flexible. You don't even have to add any extra explicit support for this. Simple allow the developer to pass in a Log instance if they with to log and, instead of appending to an internal string check for a log object, then call $this->log->log(). Only a few extra lines of code for a far more flexible logging mechanism. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=396 -- Sent by PEPr, the automatic proposal system at http://pear.php.net