Michael Gauthier [2008-11-17 03:29 UTC] Looks good. Initial comments:
1.) You should tun the code through phpcs. It will turn up several minor errors which should be easy to fix.
2.) You should build the XML with the DOM or simplexml methods. This will automatically handle escaping where it is required.
3.) Converting values to cents should go in its own method.
4.) Lastly, I find the pattern
===
if (!in_array($foo, $bar)) {
throw new Exception(...);
}
// do stuff
===
works better than
===
if (in_array($foo, $bar)) {
// do stuff
} else {
throw new Exception(...);
}
===
Daniel O'Connor [2008-11-17 04:01 UTC] Can I suggest you add in an AllTests.php so it can just slot right into our overnight unit test runs?
http://cvs.php.net/viewvc.cgi/pear/Crypt_GPG/tests/AllTests.php?view=markup is an example
Otherwise, good to see the test coverage :)
Till Klampaeckel [2008-11-17 11:56 UTC] Some feedback:
1) I'd un-tie the hardcoded dependencies in your class. For example, SoapClient(). Without making it too complicated, I'd add a public setter setSoapClient() and let people inject their own client. And if none is provided, only then create your own client.
2) private vs. protected - use protected to allow people to overload/extend your class code.
3) Use public on methods when appropriate.
4) Your Exception class needs to extend PEAR_Exception.
5) I saw you're converting to "cents" somewhere, what if the product you sell is in "cents" already? Maybe add some sort of check or configuration option to avoid the conversion.
6) Some of these vendors have a debug/test mode, if there is one, it might be a good idea to implement it. I have no idea though - this is just a thought. :-)
7) You'll need a package.xml which lists all dependencies - such as PEAR, soap, etc..
Aside from all those, looks pretty clean and very straight forward to use!
Pedro Padron [2008-12-22 16:04 UTC] Thanks for all the comments so far. Finally I've managed to dedicate some time to this package again, so here is what I've done:
About Michael Gauthier's comments:
1) You should tun the code through phpcs. It will turn up several minor errors which should be easy to fix.
The coding standards errors that are reported by phpcs aren't really errors. This question was discussed in this list a few weeks ago. You can check it out at http://news.php.net/php.pear.dev/51088 (this is the last message, check the references)
2) You should build the XML with the DOM or simplexml methods. This will automatically handle escaping where it is required.
Good idea, but not done yet. This is in my TODO list... =)
3) Converting values to cents should go in its own method.
Agreed. Now there's a _valueToCents() method.
4) Always return early
This was fixed in a class method. There was only one method with this kind of issue.
About Till Klampaeckel's comments:
1) Offer an alternative for the user to provide his/hers own SOAP client
This was done just like Till suggested. Good idea.
2) private vs. protected
Private methods are now protected. This is interesting because a Magento extension will be built based on this package. Only XmlBuilder class has private methods. I don't see why this class would be extended.
3) Public methods must be declared as public
Public methods are now explicitly declared as such
4) Package exception class must extend PEAR_Exception.
Done. PEAR_Exception > Payment_PagamentoCerto_Exception > others
5) Why converting everything to cents?
If a product is sold in cents, it should be represented as 0.40 or 0.99, for example. I have to convert to cents because that's how the API works. I really don't see a good motivation for this...
6) If the vendor provides a debug mode, use it
Unfortunately there's no such thing... I know the guys who developed the payment gateway, and I suggested this to them. We work on the same company, but I wasn't part of that project. I like PHP, so I thought about offering them some help by creating this PEAR package.
7) You need a package.xml
I already had one, but it wasn't available on the source code link I posted. Now the code is hosted at github.com, and the package.xml file is there.
About Daniel O'Connor's comments:
1) Add an AllTests.php to run all the unit tests
My next step is to increase test coverage. This is in my TODO list =)
|
