Philippe Jausions [2007-06-22 15:14 UTC] Couple of nitpicking and PEAR CS comments:
- For speed use ctype_digit() instead of preg_match(/^\d$/...)
- Constructor don't actually "return" so remove the "@return void" in docblock
- Use PEAR::loadExtension() instead of loaded_extension(), dl() and so on...
- For Crypt_DiffieHellman_Math_BigInteger add an actual factory() method to allow overriding the automatic detection of which backend to use (sometimes needed if an extension gets buggy.)
- the "class Crypt_DiffieHellman_Math extends Crypt_DiffieHellman_Math_BigInteger" is a bit an unconventional class name order. We can see the negaive side effect of this in the "if instanceof Crypt_<snip>_gmp" tests. Organize the backend classes so they are self-contained (i.e. not try to guess which type they are at runtime; if you already loaded a backend no need to recheck whether a feature is available or not.)
- Always put spaces between "if" and opening ( - same for "elseif" & co.
- Use ++$i instead of $i++ whenever possible ("for" loops)
Lukas Smith [2007-06-22 15:27 UTC] I would advise against using ctype. Its planned to be deprecated in PHP6 in favor of ext/filter, which provides similar features.
Pádraic Brady [2007-06-22 15:54 UTC] Hi Phillipe,
- I'll update the docblock here: the method allows for integers so ctype_digit() would not work without adding a further type check so integers are never passed to it. I can do this if needed.
- Constructor @return removed. Never thought about this - I'll remember it for the future.
- Would prefer to avoid a static method here unless it offers a clear advantage.
- Will check into - the Math area is a temporary set of classes.
- I'll see about adding a factory method. Probably best to link it across setBigIntegerMath() which I left public for a similar reason.
- The CS breach will be hunted down and duly squished beneath mine heel, yar!
- Not a clue why to change here (and not being sarcastic). I stopped measuring things in microseconds years ago for PHP. The entire Diffie-Hellman run probably outweighs such tiny things by a wide margin. Is it a lot slower? Was it a performance comment even?
Thanks for the feedback!
Joshua Eichorn [2007-06-22 16:32 UTC] Can you use PEAR::loadExtension in a php5 proposal, isn't that php4 code?
Greg Beaver [2007-06-22 20:50 UTC] PEAR is PHP4 code, all new packages cannot use the PEAR or the PEAR_Error class.
|
