Michael Gauthier
2009-07-03 03:52:58 UTC
Great proposal. Some suggestions:
1.) Run your code through phpcs to pick up minor whitespace and coding
style changes.
2.) CC public domain is not a recognized PEAR license. Consider using the
BSD license (or MIT, Apache2).
3.) Your proposal has great API documentation. It could be improved by
adding documentation for return values. Method docs should be second-person
imperative ("Generates a hash" rather than "Generate a hash").
4.) Parameter names should be full words (personal preference, not
required by pear).
5.) The parameter order may be improved. I can see specifying the salt
length and iteration count before specifying an actual salt value. Having
to specify the salt value as null could be annoying.
6.) The $variant parameter uses magic numbers. Consider using strings
('sha1', 'sha256', etc) instead. Magic strings are easier to read and to
remember than magic numbers.
7.) Exception parameters are not documented.
8.) Consider providing a method to get the unsupported variant from an
UnsupportedVarientException,
9.) Consider an abstract base exception class Crypt_FSHP_Exception so
users can capture any exception the package may throw.
10.) Exception class should be defined in a separate file.
11.) Methods are called statically in but are not defined statically. This
will cause a warning in PHP. Either define methods as static, or call using
$this.
12.) The 'hash' extension should be specified as a requirement in
package.xml. Even though it is built by default in PHP 5.1.2, it is
possible to disable.
13.) Since the API is so small, more extensive tests could be written.
1.) Run your code through phpcs to pick up minor whitespace and coding
style changes.
2.) CC public domain is not a recognized PEAR license. Consider using the
BSD license (or MIT, Apache2).
3.) Your proposal has great API documentation. It could be improved by
adding documentation for return values. Method docs should be second-person
imperative ("Generates a hash" rather than "Generate a hash").
4.) Parameter names should be full words (personal preference, not
required by pear).
5.) The parameter order may be improved. I can see specifying the salt
length and iteration count before specifying an actual salt value. Having
to specify the salt value as null could be annoying.
6.) The $variant parameter uses magic numbers. Consider using strings
('sha1', 'sha256', etc) instead. Magic strings are easier to read and to
remember than magic numbers.
7.) Exception parameters are not documented.
8.) Consider providing a method to get the unsupported variant from an
UnsupportedVarientException,
9.) Consider an abstract base exception class Crypt_FSHP_Exception so
users can capture any exception the package may throw.
10.) Exception class should be defined in a separate file.
11.) Methods are called statically in but are not defined statically. This
will cause a warning in PHP. Either define methods as static, or call using
$this.
12.) The 'hash' extension should be specified as a requirement in
package.xml. Even though it is built by default in PHP 5.1.2, it is
possible to disable.
13.) Since the API is so small, more extensive tests could be written.
--
http://pear.php.net/pepr/pepr-proposal-show.php?id=591
--
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
http://pear.php.net/pepr/pepr-proposal-show.php?id=591
--
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php