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

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? > > 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. Neither of which, by the way, are even close to being convincing arguments. > 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. -Melissa PS. You forgot to mark the vote as conditional in the system.