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 tilly
s 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.
Are you posting in the right place? Check out Where do I post X? to know for sure.
Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
<code> <a> <b> <big>
<blockquote> <br /> <dd>
<dl> <dt> <em> <font>
<h1> <h2> <h3> <h4>
<h5> <h6> <hr /> <i>
<li> <nbsp> <ol> <p>
<small> <strike> <strong>
<sub> <sup> <table>
<td> <th> <tr> <tt>
Snippets of code should be wrapped in
<code> tags not
<pre> tags. In fact, <pre>
tags should generally be avoided. If they must
be used, extreme care should be
taken to ensure that their contents do not
have long lines (<70 chars), in order to prevent
horizontal scrolling (and possible janitor
Want more info? How to link or
or How to display code and escape characters
are good places to start.