in reply to luke_repwalker.pl
I know you put a lot of work into this, so I am putting
some work into style suggestions back. Please take this
as constructive criticism because that is how it is meant:
- Don't use "my" for things you are using as global
variables. Instead "use vars". This gives more
context to what you mean, and the habit will avoid some
problems if you ever use mod_perl.
- Keep lines down to 80 characters. Perl has no rule
about how many lines a single line of code takes, so you
can and should break a line of code across several actual
lines for readability. This makes it easier to read and
easier to print.
- Your "scalar $#array+1" construct is redundant.
Either use ($#array+1) or use "scalar @array".
- If you are going to check parameters then use Carp,
and confess to the problem, not die. That will give you
a stack backtrace which makes the actual mistake far easier
to track down. (In general aim to have every error message
have enough information for debugging.)
- Put the expected action first. For instance in
write_file do an "or die" because you don't expect to.
(Suggestion straight from perlstyle.)
- In write_file you can and should use a hash slice
instead of writing the hash lookup 5 times.
- Personally I don't like your formatting for reasons
explained in Code Complete. Namely that it right-shifts
very fast, and it is a maintainance nightmare if anyone
changes a line of code. (Because then you have to change
several others.) Instead I suggest using whatever your
standard block indent is.
Oh, and thanks for putting so much effort in. :-)
(jcwren) RE: RE: luke_repwalker.pl
by jcwren (Prior) on Aug 06, 2000 at 07:39 UTC
|
This is perhaps a prime demonstration of several factors of releasing code too early. Most of these suggestions are excellent suggestions. A few I was aware of, but failed to act on.
I was wound up on this code. Perhaps, as merlyn would say, I had too much emotional attachment to it. I got it written, it worked, I was ready to release it. I failed to take the time to apply several things that I conciously know, or decided that it didn't warrant fixing them. As an example of code for others in the catacombs, it should set good examples. I've replied to tilly's post with my reasons or excuses (take your pick) about why certain things were done.
To make it perfectly clear, and be perfectly honest, I did not and do not take tillys suggestions as attacks on my code. I was a little mortified about a couple of them slipping past a veteran programmer, and I had a couple of 'So what? It's my code! Who *is* this guy?' thoughts to myself. But I let go, got objective, and realized that most of the suggestions were very good.
So, let's take a tour of what I did wrong, why I might have done it, and what I did about it.
- Don't use "my" for things you are using as global
variables. Instead "use vars". This gives more
context to what you mean, and the habit will avoid some
problems if you ever use mod_perl.
You're correct about this. I have a bad habit when doing quick hacks to use 'my' for globals. Corrected.
- Keep lines down to 80 characters. Perl has no rule
about how many lines a single line of code takes, so you
can and should break a line of code across several actual
lines for readability. This makes it easier to read and
easier to print.
Sorry, gotta disagree on this one. I use a modern editor that allows horizontal scrolling, and have a good sized desktop. I format my code the way I like to edit it, and that doesn't involve unnecessary line breaks. It's not my habit to print code for debugging, so I don't sweat long lines. Matter of personal style.
- Your "scalar $#array+1" construct is redundant.
Either use ($#array+1) or use "scalar @array".
Entirely correct. I think I had a keys() there before, and forgot to remove the scalar. Corrected.
- If you are going to check parameters then use Carp,
and confess to the problem, not die. That will give you
a stack backtrace which makes the actual mistake far easier
to track down. (In general aim to have every error message
have enough information for debugging.)
Some of the dies() aren't meant to produce an error messag, per se, and include the \n to prevent the line number. The rest could benefit from croak, and have been changed. In more complicated code, I probably would have used croak more liberally. As it is, this never gets very deep, and the die messages tell me enough information. But, in the interest of fostering better programing, I agree. Corrected.
- Put the expected action first. For instance in
write_file do an "or die" because you don't expect to.
(Suggestion straight from perlstyle.)
I guess it's a matter of my thought processes on this one. I think to myself "OK, we're gonna die() if...", so I write it that way. However, in most of the cases it does make more sense. Corrected.
- In write_file you can and should use a hash slice
instead of writing the hash lookup 5 times.
Not sure about this one. The print() method requires an array referrence, and to get the fields in the correct order in the CSV file requires extracting them in correct order. Perhaps I miss your point, and you could demonstrate what you mean.
Update:Added tyes patch from node 26404[] to the code, and understand what tilly was getting at.
- Personally I don't like your formatting for reasons
explained in Code Complete. Namely that it right-shifts
very fast, and it is a maintainance nightmare if anyone
changes a line of code. (Because then you have to change
several others.) Instead I suggest using whatever your
standard block indent is.
Code Complete is an excellent book. However, I do not agree with *everything* in it, and this is one of those points. I prefer heavy indention. Since I can block shift with my editor, I don't feel that I run into this problem, and am comfortable with it. Sorry. <G>
Thanks to tilly for some excellent suggestions, and for caring enough to post those suggestions. And thanks for the way they were worded. You could have been a S.O.B., and really fired some nasty comments about it. As it was, the presentation made it very clear that these were suggestions, and not "You're a moron, what are you doing writing code?" attacks.
--Chris
e-mail jcwren | [reply] |
|
$csv->print ($fh, [ $nodehash->{$_}->{nodeid},
$nodehash->{$_}->{title},
$nodehash->{$_}->{rep},
$nodehash->{$_}->{date}
]
)
# Can be written like:
$csv->print ($fh, [ @{ $nodehash->{$_} }{
qw(nodeid title rep date)
}
]
)
Note that the syntax for getting a hash slice from a reference is a bit ugly. Being able to use
$hashref->@{qw(this that other)}
has been proposed but I haven't seen a patch for it.
-
tye
(but my friends call me "Tye") | [reply] [d/l] [select] |
|
One of the nice things about dealing with good programmers
is that while they may be annoyed at suggestions, they can
tell when a suggestion is good and respond to that.
I could tell your code was good, and I am not at all
surprised that you knew most of what I could suggest.
Indeed my main reason for responding was not to give you
useful advice (though that is good), but rather to make
others reading this aware of the fact that even good code
can be improved. Also there is a difference between seeing
code that is good and being able to see why it is good,
and commenting on why code is good helps people learn
that.
As for asking who I am, well do I recognize that response
in myself. :-) Truth be told, I have less experience than you,
over fewer years, in fewer languages. But I am smart and
I try very hard to learn when I can. Hopefully at some
point I will post some code and you will will do the
same... :-)
Tye already explained how to take extract a list from a
hash, which leaves us with two issues that I would like to
mention.
The less touchy is "or die" in item 5.
Perl offers many choices here,
by all means pick the one you find matches your thought
pattern best. This is one of the cases where I sometimes
find that "unless" works for me. Mixing unless with any
and's, if's or not's lends itself to great confusion. But
an "or die" could be missed, and unless indicates that you
do not expect it to happen while if(!...) doesn't do that
for me. So TIMTOWTDI, pick what matches your mind.
And now for the controversial layout question. As we both
know, layout is one of those things that people develop
very strong opinions on. So let me describe where mine
come from.
I used to
feel as you do. I read Code Complete and came out saying
the same thing. As long as it fit on one line in my editor
on my nice screen, I was fine.
Then one day a co-worker popped my code up in vi in his
machine where real-estate was being used for lots of
terminals in fairly large fonts. (He doesn't have the best
eyesight in the world.)
My jaw fell open and I immediately apologized.
Literally the first thing I did when I went back to my desk
was I found a way to put a line at 79 characters on my
screen and I began paying attention to it.
You may disagree. In fact I know of good reasons to
legitimately
disagree. As we both know there is huge value to being
able to put as much code on screen at the same time as
possible. On your computer, for you, your layout does
that. As soon as code goes out of sight it is
out of mind and bug counts rise.
I understand, and indeed if anyone finds an
easy way to split X so that it works like one window that
is half as wide and twice as long, please tell me. I
would use that in a heart-beat. :-)
Now you like your layout because you see it in your modern
editor on your huge screen. Well I am seeing it in a
crappy browser on a laptop chosen for its form factor.
Did you want me to be able to read it? :-)
Cheers,
Ben
| [reply] |
|
|