Re: [PEPr] +1 for Web Services::Services_Libravatar

On Sat, Jun 25, 2011 at 11:06 AM, Melissa Draper <melissa@meldraweb.com> wrote: > On Wed, Jun 22, 2011 at 1:58 AM, Till Klampaeckel <till@php.net> wrote: >> >> Till Klampaeckel (http://pear.php.net/user/till) has voted +1 on the >> proposal for Web Services::Services_Libravatar. >> >> Proposal information: >> http://pear.php.net/pepr/pepr-proposal-show.php?id=658 >> Vote information: >> >> http://pear.php.net/pepr/pepr-vote-show.php?id=658&handle=till >> >> Comment: >> >> I second Markus' suggestions. Overall, nice code. >> >> Here are my additions why my vote is conditional: >> >> 1) Your methods in their current form are way to long. I think you can >> break them up into multiple methods, and also refactor your if/else >> structures to exit early. >> >> 2) I'd also rename url() to getUrl() (no params), and I'd use a >> __construct($identifier, array $options = null) to initialize the class. >> >> 3) For the sake of re-using this object, each option provided to the >> constructur should have a set-method associated. Maybe setOptions() and >> setIdentifier()? > > > So basically you're telling me /now/, after an *entire month* of RFC, that I > should rewrite the whole thing. The object can already be reused. > What the lol? None of us are paid here to do code-review and sorry if this is late or too late for you. You can of course disagree with my recommendation, but I resent your tone. ;-) This is not meant to be personal against you or the code you wrote, I'm just reviewing the code. That's all. I'm also offering my help, if you want it. >> >> 4) srvGet() - or rather getSrvRecord() - should utilize Net_DNS2. >> > > > Oh for deity's sake. Why? > I'm yet to see a convincing argument as to why I should add a dependency on > an additional library to handle what is provided in *core php* > by dns_get_record. That is, other than to please those who think an EOL'd > php version for Windows servers is a valid SOE, or dependency for dependency > sake. Then strictly speaking, we all shouldn't be using any libs at all since it's all possible with core PHP. Right? Or maybe not. (I'm being sarcastic, of course.) > Neither of which, by the way, are even close to being convincing arguments. The one and only reason is seperation of concerns. >> >> 5) I'd really, really like to see a couple unit tests. Let us know if we >> can help you. > > > I'm keen to hear how to reliably test against real-world srv responses for > something of this nature. Using a mock. A simple example in PHPUnit: $mock = $this->getMock('Services_Libravatar', array('getSrv')); $mock->expects($this->any())->method('getSrv')->will($this->returnValue('something to return')); That's IMHO the easiest way to keep it a unit test and to run the code without setting up a DNS server/resolver. A couple tests are badly needed, because for example, the code right now might even return a 'null' if I read it correctly. If it doesn't exit in the last loop, it's null. So does identifierHash() btw.. I'm pretty sure I can break it. Or the code using it. Tests help all of us to setup a clear expectation of the code. ;-) I can fork your repo and commit some examples to illustrate what I mean - if you want to. >  -Melissa > PS. You forgot to mark the vote as conditional in the system. No, I didn't. :-) Apparently there's a bug in the system. Cheers, Till