In http://blog.simon-cozens.org/blosxom.cgi/2002/Sep/16#6255, Simon muses about programming without variables, and how everything looks nice and clean.
I've been there too. Except now I err on the side of using variables. Why? Because my personal debugging technique is to dump stuff to STDERR and examine it. I never ever ever use the perl debugger. So in order to be able to insert random debugging events here and there I generally have to use variables.
At least that's my excuse ;-)
Ugh. What happens when it doesn't match? Apparently Simon has not been reading MJD's Program Repair Shop closely enough.shift->TicketObj->Subject =~/^Perforce file (.*#\d+) for review/;
print SSH $1, "\n";
Didn't Simon write a Beginner's Perl Book? Maybe I should distrust more and more of that book.
Re:Ugh. Bad Code!
Simon on 2002-09-17T17:44:43
Ugh. What happens when it doesn't match?Bzzt! That can't happen.
Notice the sub calledIsApplicable? That's tested before theProcesssub is called.But I suppose you couldn't pass up an excuse to trash a competing book.
Re:Ugh. Bad Code!
merlyn on 2002-09-17T18:13:06
There's no such thing as "that can't happen", and no excuse for at least adding "|| die" to that statement so that at least if the match didn't happen, you wouldn't be using a stale $1.Geez.
Re:Ugh. Bad Code!
petdance on 2002-09-17T22:41:46
or even an assert_defined($1) if you're using Carp::Assert::More.I have to agree with Randal, and I'm surprised that Simon would say "that can't happen".
Re:Ugh. Bad Code!
merlyn on 2002-09-17T23:46:40
No, that's insufficient. If the match fails, you get the previous value of $1, which is why this is such an insidious bad idiom, and even a potential security hole.or even an assert_defined($1)You must test the result of the match to see if it matched or not.
Re:Ugh. Bad Code!
petdance on 2002-09-18T01:12:27
True enough. And y'know, I was just thinking about this last night while I was reading Friedl's 2nd edition of Mastering Regular Expressions, too...Re:Ugh. Bad Code!
brian_d_foy on 2002-09-17T22:55:32
Forget the || die. Simon could have written just as easily:
print SSH $1 if $foo =~/.../;
And, for those of you who might think Randal is being harsh, well, try pair programming with him. You guys have it easy. He holds Stonehenge instructors to an even higher, higher standard (and I thank him for that).
We all write bad code. It's how we act when someone points it out that matters.:) Re:Ugh. Bad Code!
merlyn on 2002-09-17T18:23:56
(Sorry, this should have gone in the other post.)And, it's not so much that you have a "competing" book, except that I mentioned that because I hold anyone who teaches (or writes for) beginners on a regular basis to a higher standard.
Since you have made yourself one of those people, you get judged more critically by me. You have a higher responsibility to write more robust and cleaner code, and counter-examples will be pointed out in that context because it discredits your consistency of mission.
At least when I write bad code, I try to say "this is bad code". {grin}
Re:Ugh. Bad Code!
ask on 2002-09-19T00:49:50
Say, with this code (I know it could be written better, but for the sake of the example...)
if (m/\w+\d+\w+/) {
s/(\d+)//;
print $1, "\n";
}
Would you put a double extra check on the s///?
Then, can't you reasonably say "it won't fail!"?
I think you can, sometimes.:-)
(and as someone else pointed out, Simon could easily rewrite the code to do the check without an extra variable; I agree that the calls to open, print to and close SSH should have error checking though).
- askRe:Ugh. Bad Code!
jplindstrom on 2002-09-20T21:44:11
"Then, can't you reasonably say "it won't fail!"?"
Not right now. But after three changes to the first regexp, and the second regexp being pushed down a few lines (so it´s not in proximity to the first), it´s not that obvious anymore, right?
It´s far fetched (and maybe not worth fuzzing about), but not _that_ far fetched:)
/J
