Joe Stump [2008-04-28 23:55 UTC] 1.) I'm unsure if PEAR requires separate files for each exception (I saw a recent proposal that had all of them in one file), but I'd prefer to see a single Exception.php file with all exceptions in there or a lazy loader of exceptions rather than three separate include files (each requiring an fstat and fseek along with a loop through the include_path).
2.) By using nothing but private members/methods there's no way for me to do much of anything by way of extending or mocking up of the object. Travis Swicegood may want to chime in more here as he's the one who broke me of this habit, but I generally prefer protected over private these days.
3.) I have a slightly different pattern that I use for setters if you'd like to ping me offline I'll give you more details. I generally dislike the use of tons of setters now that PHP5 has __set() and I think there's a way to offer the flexibility you're looking for while not having a setter for each member variable (Daniel has my contact info).
4.) Why not use __call() for the example? e.g. $client->SetExpressCheckout();
5.) In the call() method you throw $e, which is a SoapFault. I generally, when coding with PEAR, expect a package related exception to be thrown. Maybe add an unknown exception type and rethrow that with the message, etc.? Dunno. Just a thought. Not sure if the docs cover throwing non-package related exceptions.
6.) I think I'd change the package name. To what I don't know, but my top suggestions are: Payment_PayPal_SOAP and Services_PayPal_SOAP. I'd expect there to be other Payment_PayPal related packages in the future so it'd make sense that this be a "sub-package" of that maybe.
7.) There's no ways for me to modify any of the SOAP stuff before the SOAP client is loaded in __construct(). This is an argument for one of the following: 1.) Make the $_soapOptions array protected so I can override it in my child class (another argument for #2), 2.) Lazily load the SOAP client only when call() is ran along with a way to manipulate that data via a setOption() or some such method.
I like the package. Concise and simple. Just the way I like my code :) Ping me offline about my setter pattern if you're interested. Thanks!
Joe Stump [2008-04-28 23:58 UTC] 8.) Unit tests are needed. Preferably one for each call so I can a.) have an example and b.) we know every call works as expected. (I can hear Michael hating me all the way from the other coast right now)
Michael Gauthier [2008-05-05 13:34 UTC] Joe, thanks for the in-depth feedback. I agree with and have addressed most of your points in an update to the proposal. The two remaining points you brought up are:
3.) I'm still not 100% happy with the way getters and setters are handled in this class. I'd like to get this cleaned up before a vote.
4.) Initially, I used __call() instead of call(). I changed because
a.) __call() should be private and is then very difficult to document, and
b.) __call() takes a variable number of arguments and we only want one argument for PayPal requests. This means I have to validate the number of arguments if I use __call().
Joe Stump [2008-05-05 15:42 UTC] Michael,
The updates are looking good. I'm still not a fan of private variables, but changing those at a future date to protected won't, technically, break BC so I've got time to sway you on that. :)
1.) You throw InvalidArgumentException in the client's constructor. I don't see this defined anywhere. I see references on the Googles to it being in SPL. Unsure what PEAR's stance on SPL is at this point.
2.) In call() I'd probably type hind $arguments as `array $arguments = array()`.
Other than that everything looks good. Thanks!
--Joe
|
