Vote Details for "Services_ProjectHoneyPot" by tswicegood
| » Details |
|---|
|
| » Comment |
|
This vote is conditional, but so long as these conditions are implemented prior to a v1.0 stable, I'd be happy. * add setResolver() that checks for an instance of a Services_ProjectHoneyPot_DNSResolver interface or the Net_DNS_Resolver class to allow people to reuse their own DNS resolver class so long as they write an adapter that implements the expected interface * move the array in the in_array() call in setResponseFormat() to its own protected property. Saves memory by not re-creating an array on each call and allows someone to add their own valid types by subclassing Services_ProjectHoneyPot * in the Response object, move toward a Strategy implementation with each portion of the if() {} being implemented in its own object to allow for custom return types. Names of those objects should be determined by convention rather than mapping (i.e., the type "array" resolves to Services_ProjectHoneyPot_Response_Types_array rather than having an internal map array in the Response object that handles mapping 'array' to a specific class name). This allows new types to be added in without having to adjust the Response object. * Looking at the "object" and "array" response types, I would recommend refactoring their instantiation so you always instantiate the object type, then add a toArray() method to it to allow returning just the array. In the above implementation, the ..._Types_array object would use the ..._Types_object class to create and return the object, then just do a "return $result->toArray();" to coerce it into an array prior to returning. -- end of conditionals -- On a style note, I'm not a fan of the use of a factory() in this case as there's no clear benefit that I can determine. Static methods are (currently) the equivalent of a namespaced function. As there is no ability to dynamically call them (prior to 5.3/6.0), they hinder polymorphic behavior by forcing more verbose code to allow swapping out which class is instantiated. Personally, I don't see any benefit in adding the factory() method over using a standard constructor and as such would remove it. |