use Perl

• Ugh. Bad Code! (Score:0, Troll)

  by merlyn (47)

      shift->TicketObj->Subject =~ /^Perforce file (.*#\d+) for review/;
      print SSH $1, "\n";

  Ugh. What happens when it doesn't match? Apparently Simon has not been reading MJD's Program Repair Shop closely enough.

  Didn't Simon write a Beginner's Perl Book? Maybe I should distrust more and more of that book.

  --
  

  • Randal L. Schwartz
  • Stonehenge
  • Re:Ugh. Bad Code! (Score:1, Flamebait)

    by Simon (89)

    Ugh. What happens when it doesn't match?

    Bzzt! That can't happen.

    Notice the sub called IsApplicable? That's tested before the Process sub is called.

    But I suppose you couldn't pass up an excuse to trash a competing book.
    • Re:Ugh. Bad Code! (Score:2)

      by merlyn (47)

      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.

      --
      

      • Randal L. Schwartz
      • Stonehenge
      • Re:Ugh. Bad Code! (Score:2)

        by petdance (2468)

        or even an assert_defined($1) if you're using Carp::Assert::More [perl.org].

        I have to agree with Randal, and I'm surprised that Simon would say "that can't happen" [perl.org].

        --
        

        --
        xoa
        • Re:Ugh. Bad Code! (Score:2)

          by merlyn (47)

          or even an assert_defined($1)

          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.

          You must test the result of the match to see if it matched or not.

          --
          

          • Randal L. Schwartz
          • Stonehenge
          • Re:Ugh. Bad Code! (Score:2)

            by petdance (2468)

            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...

            --
            

            --
            xoa
      • Re:Ugh. Bad Code! (Score:1)

        by brian_d_foy (44)

        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! (Score:2)

      by merlyn (47)

      (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 l

      --
      

      • Randal L. Schwartz
      • Stonehenge
      • Re:Ugh. Bad Code! (Score:2)

        by ask (83)

        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).

        --
        

        -- ask bjoern hansen [askbjoernhansen.com], !try; do();
        • Re:Ugh. Bad Code! (Score:1)

          by jplindstrom (594)

          "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