RE: luke_repwalker.pl

[tilly]

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:

1. 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.
2. 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.
3. Your "scalar $#array+1" construct is redundant. Either use ($#array+1) or use "scalar @array".
4. 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.)
5. Put the expected action first. For instance in write_file do an "or die" because you don't expect to. (Suggestion straight from perlstyle.)
6. In write_file you can and should use a hash slice instead of writing the hash lookup 5 times.
7. 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]

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.


1. 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.
   
   
2. 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.
   
   
3. 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.
   
   
4. 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.
   
   
5. 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.
   
   
6. 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.
   
   
7. 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

[tye]

I'll jump in. Here is how to use a hash slice:

      $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")

[tilly]

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