Roman Neuhauser wrote: > # 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. 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
