Re: [PEPr] +1 for Images::Color2

I apologize in advance if this shouldn't be sent to the list, not sure where else to note my response. On 30 Sep 2005 16:15:12 -0000, Justin Patrin <papercrane@reversefold.com> wrote: > > Justin Patrin (http://pear.php.net/user/justinpatrin) has voted +1 on the proposal for > Images::Color2. > > Proposal information: > http://pear.php.net/pepr/pepr-proposal-show.php?id=297 > Vote information: > > http://pear.php.net/pepr/pepr-vote-show.php?id=297&handle=justinpatrin Thanks Justin, I really appreciate the time you put in to review the code. You caught several things I'm a bit embarased to have missed. > This vote is conditional. The condition is: > > Do not use @ to silence errors/notices due to missing array keys. Make them required or use > isset(). (seen in the constructor for the main class) Done. I wasn't sure about it at first but it actually cleaned up the code. > Use " instead of ' for strings which don't have special chars or variables in > them. (as in Hsl.php fromString()) Done. > Use the PEAR_Exception class (assuming that it has been officially comitted). I'll look into this. > You're using floatval() in some places and (float) in others. Please use one or ethe > other. Personally I like (float) but it's your call. (Hsl.php uses floatval()) Eck, I didn't notice that. I've changed it to use (float). For some reason I though floatval() worked better with strings but I can't find any documentation for that and my unit tests pass just fine with (float). > Only use printf functions when it is absolutely needed. printf is always slower than > concatenation and is also slower than using "" and interpolation. Use of sprintf() in > Hex.php is ok as it does further formatting (%02x) but not in Hsl.php which uses %d. %d can be > replaced with concatenation and (int) if you need it. Done. In an earlier version the string formats needed printf(). I thought I'd changed them all over. > Please put spaces around the => operator. Done. > (Optional) > > Use ' and . instead of " for strings. Putting variables inside "" strings > is less efficient and harder to notice. Yeah, I hear the speed but it just looks cleaner. On the speed front, the only place I'm really using them is when exceptions are thrown so it shouldn't affect performance. Regarding the visual aspect, when I'm thinking about it I wrap them in {} to make them more obvious. > Why use is_null instead of === null? It doesn't work. I couldn't remember if I was just doing it out habbit but I tried the === null and tests started failing.