# jochem@xxxxxxxxxxxxx / 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. 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 } 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 } 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 } 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 } 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. -- How many Vietnam vets does it take to screw in a light bulb? You don't know, man. You don't KNOW. Cause you weren't THERE. http://bash.org/?255991 -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
