Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.

Code Which Gives Me Pause 25 Comments More | Login | Reply /

 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • I'm inclined to go for something more like:

    my $country = $card->country;
    $country = (!$country || $country eq 'gbr') ? '' : uc "[$country]";
    Unless the difference between undef and an empty string matters.
  • ...on where the vars are used. In that example, it looks like the $country scalar is only used for the check. But I might do it like this:

    if (my $country = $code->country) {
      $country = ( $country eq 'gbr' ? '' : uc "[$country]" );
    }
    .phaylon
    --
    Ordinary morality is for ordinary people. -- Aleister Crowley

  • my $country = uc $card->country || '';
    $country = ($country =~ /^$|GBR/ ? '' : "[$country]");
    Possibly ^GBR$.

    • Yuck. Using a pattern match to test string equality is smelly.

      • I can think of two reasons.

        One is ugly syntax, which I find a silly argument. All Perl code is ugly, but elegance is more important than beauty. And I think this is very elegant.

        The other is that you can only have 2_501_710 such matches per second (on my PM 1,5 GHz laptop, the average of 50% matches and 50% non-matches), which can be a valid argument in some cases. But if performance is the issue here, the method call should go first: my laptop can do just 1_415_988 method calls per second, and that's on an

        • I am honestly somewhat alarmed that you could only come up with straw men. How about “a regex match always has many more subtleties and edge/corner cases than straight string comparison”?

          As soon as I saw your code I wondered: “Did he mean ^GBR$ or was the omission of anchors deliberate?” And next: “Hmm, he uppercased it but there’s no /i – was that on purpose?”

          I prefer to minimise the number of speed bumps that a casual reader of the code has to pass.

          • As for the omission of anchors: probably a bad idea. I considered this and added a remark about it to the post. OTOH, I frequently have code like:

            $lang =~ /nl|en|de|eo/

            if I know $lang will always be 2 characters. Here, I didn't know, and should have used anchors.

            The case sensitivity has nothing to do with regexes. With $foo eq 'GBR' you can (should) ask yourself the same question: shouldn't uc($foo) eq 'GBR' be used instead? However this is somewhat irrelevant here, as the line just before the match, an

            • I’m not advocating baby-talk code like that.

              Note that neither of the variants you suggest preserves undefs/zeroes. For that case, I gave a much simpler version that folds both conditions into a single ternary.

              I don’t see how putting two conditions inside a regex as alternation is abstraction. You don’t have two Perl expressions, but instead a regex with two match possibilities. The complexity hasn’t gone anywhere, it’s still there right in front of the reader’s eyes,

              • I am indeed assuming that a method called "country" will not return 0, and that undef is to be equal to "". The addition of square brackets, and the complete omission of the country indication when it's one specific value, tells me that this will be used in another string, meant for end users.

                The complexity is still there entirely, but written in a more compact way: there's only one character in between. This makes it easier --for me-- to notice that in practice, both conditions lead to the same thing.

                I don

                • OK, being alarmed is hyperbole... to an extent. This particular example does not warrant it, but on general principle I *would* be worried if the only reasons you could come up with for preferring string comparison over regexes are performance and ugly syntax.

                  Compare:

                  my $country = uc $card->country || '';
                  $country = ($country =~ /^$|GBR/ ? '' : "[$country]");

                  With:

                  my $country = $card->country;
                  $country = ( $country and $country ne 'gbr' ) ? uc "[$country]" : '';

                  Regardless of how you turn it,

  • (but I can't resist anyway)
    (my $country = $card->country || '') =~ s/^(?!gbr)(.+)$/\U$1/;

  • … only I’d write the conditional the other way around:

    my $country = $card->country;
    $country = ( $country and $country ne 'gbr' ) ? uc "[$country]" : '';

    • Oops, that’s not actually the same. Well, it might be OK; if zeroes and undefs are not valid values that you need to care about, then that is how I’d write it.

      If you do need to distinguish, though, then I’d put it this way:

      my $country = $card->country;
      $country
          = not $country          ? $country
          :     $country eq 'gbr' ? ''
          :                     

      • As for speedbumps... nested ternaries really slow /me/ down! (Except for simple 1-on-1 mappings.)

        • Did you miss the part where I said this complex solution is only necessary if undef and zero need to be preserved? Sure this one is complex – because a three-way condition is always complex.

          • It is never necessary. Every such nested ternary can be written more verbosely, although you may have to use temporary variables.

            my $country = $card->country;
            if ($country) {
                $country = uc "[$country"];
                $country = "" if $country eq "[GBR]";
            }

            (I swapped the uc and gbr check around, because it makes the code prettier IMO.)

            Though what's really needed in this code, regardless of the Perl syntax used, is a comment stating its intention. Perhaps:

            # Don't mention the country if it's u

              • Yes, I did. But I would pick such a verbose style over short ?: tables.

                As for comments: they provide redundancy. When the comment and the code are no longer in sync, that's a good sign something's wrong, and that helps to write good code.

  • My guess is that sometimes, $country is undef. And because it might be, the author writes horribly perverted code just to avoid getting an irrelevant warning message. This is the downside of warnings.

    And given that, I'd have written this code as: $country = $country eq 'gbr' ? '' : uc $country; See how much clearer it is when you stop worrying about stupid warnings? Don't be pedantic about warnings. Stop using them when they make your code look ugly. (/end rant).

    --
    • Randal L. Schwartz
    • Stonehenge
    • My interpretation....was that $country wasn't dealing with definedness, but was instead determining whether or not to use brackets. If there's nothing there, "[]" might look stupid, so you don't want to put the brackets; and if it's 'gbr', you don't want anything there for whatever reason.

    • If it’s really just undef, then an or '' in the appropriate spot would suffice – no need for convolution.

      That said, almost all my code starts like so:

      use warnings;
      no warnings qw( once qw );

        • Sometimes you refer to a variable in another file:

          $CGI::DISABLE_UPLOADS = 1;

          That snippet will prevent a DOS on your server from folks repeatedly uploading files. However, it's probably only in your code once and warnings will issue a warning about it which you can disable with no warnings 'once'.

          It's designed, however, for cases where you declare something and never use it again. That's probably cruft that you don't actually want in your code (or worse, you declare something but refer to it by the

  • my $country = $card->country;
    $country ||= $country eq 'gbr' ? '' : uc "[$country]";

    SCNR… :-)

    • my $country = $card->country;
      $country    = ''              if $country eq 'gbr';
      $country    = uc "[$country]" if $country;

      Keep it simple.

  • I'd refactor the Card class so that the default value for the getter was "GBR" and the setter automatically uc'd its argument. With that in place:

    my $country = $card->country eq 'GBR' ? '' : "[$card->country]"
Stories, comments, journals, and other submissions on use Perl; are Copyright 1998-2006, their respective owners.