Roman Neuhauser wrote:
> # [EMAIL PROTECTED] / 2007-01-12 01:57:27 +0100:
>> Brian P. Giroux wrote:
>>> If anyone can help me out with that or provide any other advice about
>>> the rest of it, I'd be grateful.
>>> The file can be found at http://www.senecal.ca/normnums.php.txt
>
>> keep commenting all your code to that extent! you do us proud :-)
>
> I find the *inline* comments superfluous and cluttering the code.
Because of my inexperience, I need the comments to remind me of what I
did a few days prior.
> They don't add any insight, they simply repeat what the code already says
> very succintly:
>
> 1 function is_valid_isbn10_check_digit($cd) {
> 2 // check if th function was passed only a single character
> 3 if(1==strlen($cd)) {
> 4 // check if the digit is a valid ISBN-10 check digit
> 5 if(is_numeric($cd) || 'x'==$cd || 'X'==$cd) {
> 6 return true;
> 7 } else { // the digit is invalid
> 8 return false;
> 9 }
> 10 } else { // the check digit isn't 1 character
> 11 return false;
> 12 }
> 13 }
>
> Comments on lines #2, #4, #7, #10 only restate painfully obvious things.
> Who needs to explain that "13 == strlen($ean)" checks that the length of
> $ean is 13?
>
> This shorter version is more readable for me:
>
> 1 function is_valid_isbn10_check_digit($cd)
> 2 {
> 3 return (1 == strlen($cd)
> 4 && (is_numeric($cd) || 'x'==$cd || 'X'==$cd)
> 5 );
> 6 }
WOW! I knew the functions could be whittled down (although I couldn't
see how) but I never thought it could be just be a single statement.
> The code is quite complicated for no good reason I could see:
>
> 1 function is_valid_ean($ean) {
> 2 //check that the string is 13 characters long
> 3 if(13==strlen($ean)) {
> 4 // make sure all digits are numeric
> 5 if(is_numeric($ean)) {
> 6 if(0==digit_sum($ean,1,1,3)%10) {
> 7 return true;
> 8 } else { return false; }
> 9 } else { return false; }
> 10 } else { return false; }
> 11 }
>
> First step:
>
> 1 function is_valid_ean($ean) {
> 2 if(13==strlen($ean))
> 3 if(is_numeric($ean))
> 4 if(0==digit_sum($ean,1,1,3)%10)
> 5 return true;
> 6 return false;
> 7 }
This makes sense to me, I had forgotten that "return" exits the function
immediately, making the "else" statements unnecessary.
> Second step:
>
> 1 function is_valid_ean($ean) {
> 2 if(13==strlen($ean)
> 3 && is_numeric($ean)
> 4 && (0==digit_sum($ean,1,1,3)%10))
> 5 return true;
> 6 return false;
> 7 }
This also makes sense now that the "else" statements have been removed.
> Third step:
>
> 1 function is_valid_ean($ean) {
> 2 return (13 == strlen($ean)
> 3 && is_numeric($ean)
> 4 && (0 == (digit_sum($ean,1,1,3) % 10))
> 5 );
> 6 }
Again, WOW! This is certainly the version I will use (if you don't mind).
> The last version tells me what I need to know, and tells it only once!
> The three lines are so little of so "uninteresting" code, (there's
> obviously nothing overly complicated going on) that they don't need more
> explanation than a good function name provides.
>
--
Brian P. Giroux
--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
