Vote Details for "Services_ShortURL" by gauthierm
| » Details |
|---|
|
| » Comment |
|
Well written. For the convenience of others, this package is BSD licensed, not PHP licensed as the proposal indicates. Couple of feedback points: 1.) cURL extension is no required or optional in package.xml but HTTP_Request2 is created with cURL by default. 2.) Why explicitly use cURL by default? Why not let HTTP_Request2 decide that one? 3.) The accept() method is confusing to me. Why not setRequest(). Also, you could use type hinting here rather than checking the parameter type manually. The only reasons I can think of for doing it this way are if you are implementing a generic interface but that does not seem to be the case. 4.) License tag links shouldn't use tinyurl because links could disappear in the unlikely even that tinyurl disappears. 5.) That's all I've got. Looks great. |