Comments for "Crypt_DiffieHellman"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • 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.