RichardOnRails
11/21/2007 2:25:00 AM
On Nov 19, 2:23 am, Ryan Davis <ryand-r...@zenspider.com> wrote:
> I realize you solved your problem, but there is a much more insidious
> underlying issues here:
>
> On Nov 18, 2007, at 22:40 , RichardOnRails wrote:
>
> >> MLN = 2 # MaxLeadingNumbers
>
> MLN
>
> >> sName =~ /^([\d]+)?\.?([\d]+)?\.?([\d]+)?\.?/
>
> sName
>
> >> n = Array.new
>
> n
>
> >> print "DBG0>> "; (1..MLN+1).each { |i|
> >> printf("\t#{n[i]}") };
>
> nasty compounded lines and obtuse debugging output
>
> >> i_thNumberExists = n[i] ? true : false
> >> iThNumbrerHasLeadingZero = i_thNumberExists &&
>
> i_thNumberExists
> iThNumbrerHasLeadingZero
>
> ARGH! My eyes! STOP!!!
>
> Ruby is NOT C.
>
> For the love of all that is ruby, please write readable code!
>
> Use English variable names (or whatever your native language is--just
> use words). Don't use hungarian notation, it doesn't help--hell, your
> variable names are so ugly you can't even read the misspellings in
> them. Don't mix camelcase and underscores. Don't use 'n[i] ? true :
> false' when 'n[i]' would do. Don't debug with logging. Write tests. /
> [\d]+/ is the same as /\d+/. Those evals are TERRIFYING. Don't use them.
>
> And, most importantly, 2 spaces per indent.
>
> If you follow these suggestions, your code will be much more
> understandable and less error prone. Really.
Hi Ryan,
Thanks for taking the trouble to critique my code. Since you were
kind enough to take a thorough look at the code and offer detailed
comments, I feel obligated to address each issue you raised.
I agree with some of your responses, don't understand some, and
suspect some are merely a matter of taste. I'm going to tackle them
one-by-one, but I especially want to thank you for suggesting Unit
Test in Ruby, which I haven't used yet (and don't immediately see how
I'd apply them).
> >> MLN = 2 # MaxLeadingNumbers
>
> MLN
Are you suggesting that rather than using an abbreviation for
MaxLeadingNumbers, I should use the longer string? If so, don't you
think that would create unnecessary clutter? Is this more than a
matter of personal taste?
>
> >> sName =~ /^([\d]+)?\.?([\d]+)?\.?([\d]+)?\.?/
>
> sName
Did you draw attention to this because of the Hungarian notation? If
so, do you think I'm unwise to adopt the style once advocated by
Charles Simoni, super-programmer and co-founder of a giant software
company?
> >> n = Array.new
>
> n
>
> >> print "DBG0>> "; (1..MLN+1).each { |i|
> >> printf("\t#{n[i]}") };
>
> nasty compounded lines and obtuse debugging output
There was one more piece of that line that got cut off somehow:
"puts". If we get aside from the tracing issue for a moment, don't
you think it's better to have one-liner debugging statements that can
be easily deleted later, rather than multi-line statements where
something might be overlooked when deleting them later?
> >> i_thNumberExists = n[i] ? true : false
> >> iThNumbrerHasLeadingZero = i_thNumberExists &&
>
> i_thNumberExists
> iThNumbrerHasLeadingZero
To me, "i_thNumberExists" reads naturally in English as "i'th Number
Exists". I stated out with iTh ... and thought it less readable. I
eventually replaced all instances of iTh, but posted my code before I
completed that correction. My apologies; I agree that had I not made
them uniform, they would provide risks of misspelling.
> Ruby is NOT C.
I'm a retired software developer with experience with a lot of
languages, but by far the most experience with C and C++ under
Windows. So it takes a little while to adopt the idioms of new
language.
> Use English variable names (or whatever your native language is--just
> use words). Don't use hungarian notation, it doesn't help
> --hell, your
> variable names are so ugly you can't even read the misspellings in
> them. Don't mix camelcase and underscores.
I addressed those issues above.
> Don't use 'n[i] ? true : false'
> when 'n[i]' would do.
Agreed. In this case, I wasn't sure that Ruby was honoring that
equivalence, so I wanted to make it explicit.
> Don't debug with logging. Write tests.
GOOD POINT!
> /[\d]+/ is the same as /\d+/.
Thanks.
> Those evals are TERRIFYING. Don't use them.
How else can you loop through $1, $2 ... without repetitive code?
> And, most importantly, 2 spaces per indent.
Agreed. I thought I did that when I posted.
> If you follow these suggestions, your code will be much more
> understandable and less error prone. Really.
I'll keep my fingers crossed :-)
Regards,
Richard