our $DEFAULT_COUNTRY = 'gbr';
[...]
my $country = $card->country;
if ($country) {
    if ($country eq $DEFAULT_COUNTRY) {
        $country = '';
    } else {
        $country = '[' . uc($country) . ']';
    }
}
[download]

And the uc() looks like maybe a display thing that should be in a template somewhere.

I didn't mention it, but this is in a Template Toolkit plugin, so this is a good spot for that behavior. Otherwise, that would have been a fantastic catch.

Cheers,
Ovid

New address of my CGI Course.

my %ignore = ( gbc => 1);

my $country = $card->country;
if ($country) {
    $country = $ignore{$country} ? '' : uc($country);
}
print $country;
[download]

Since one reads code left to right, there is a tension between the brevity you gain from statement modifiers and the cognitive burden of having to mentally rewrap the preceding statement with a conditional. Backtracking to wrap a conditional around another conditional is a bit much IMHO.

-guinex

One does, other doesn't. Just like I do not see any reason to rewrap "Stay at home if it rains." to "If it rains, stay at home." I do not see the need to rewrap statement modifiers.

Jenda


Support Denmark! Defend the free world!

The issue is the other conditional.

If the dog is barking let him in otherwise give him a bone, but only if it is raining.

This sentence reads nicer if we place the "if it is raining" earlier in the sentence. I however, would force the truth of $country

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

Unfortunately, this breaks when we need to check definedness (until we have a reliable // operator). In that case (assume "0" is a valid country) I would probably go with something like:

my $country = $card->country;
$country = (!defined($country) or $country eq 'gbr') ? '' : uc "[$coun
+try]";
[download]

Which is just as complex as the original, but the full conditional is easier to find.

Good Day,
    Dean

An alternative might be:

my $country;
if ($country = $card->country) {
    $country = $country eq 'gbr'
             ? ''
             : uc "[$country]";
}
[download]

Since we're nitpicking a bit, I'll mention that I was wondering when this layout of the ternary operator would appear :)

I can understand that argument, but whenever you're describing business rules in code, you should be very clear to separate each business rule check from each of the others.

In this case, it's pretty obvious that there are two separate business rules in play: (1) that 'gbr' is a special overriding case, and (2) that the template must show countries in a particular format.

If either one of those business rules were to be changed by the boss, then the code involved would have to be restructured anyway. Maybe the 'gbr' isn't the only special case next year. Maybe the template will need something more complicated next month. Maybe 'gbr' will still be a special case but its appearance should be derived from the usual templating format, not just a completely empty string. So why invite a headache at that point, when you can just write things more clearly delineated and flexibly now?

The group I am in right now tries to be very careful and makes the code auditable. We are supposed to tag various code with the exact paragraph-number in the requirements document that the code is intending to implement. Many companies don't need to go that far, but they do prefer a bugfix patch to include the exact bug-number in the tracker database.

$country = $country eq 'gbr' ? '' : uc "[$country]"
  if $country;
[download]

Ease of read vs clear to reflect business logic should not happen. They are not mutually exclusive, they should happen at the same time. I think, however, that if a code is clear to reflect business logic, it's easy to read, but not vice versa.

Well, If I had to rewrite the code (not the overall logic or even redesign the flow), I would write:

if ($country) {
  $country = $country eq 'gbr' ? '' : uc "[$country]";
}
[download]

my $country = '';
my $code = $card->country;
$country = "[\U$code]" unless $code eq 'gbr';
[download]

Update: I should add that the third line potentially generates warning, but I assumed that the country() method always returns defined value. And yes, I agree that the comparisson value (gbr in this case), should be abstracted out.

I wonder if you mean "too much in a line" or "too much in a statement"? We all know Perl doesn't mind newlines in a statement, so my personal rule that "seventy columns in a line is way too many" can help make the statement more comprehensible. Just add sensible phrasing and indentation.

The ". . . if $country" modifier can be removed, and the hard-coded 'gbr' with it, if we keep a hash of exceptions:

my %oddball = (
    ''  => '',
    gbr => '',
);

my $country = $card->country;
$country =
    exists $oddball{$country} ?
    $oddball{$country} :
    "[\U$country\E]";
[download]

If you hadnt done it first I would have posted more or less the same thing.

Although the evil part of me did think of writing a tied hash for this :-)

---
$world=~s/war/peace/g

I like this approach, too; the hash makes exceptions to the "uppercase bracket" basic rule perfectly clear. However, I also agree with other posters (1) treating $country as a temporary variable isn't great practice, and (2) it's not entirely clear what $card->country is allowed to return. Given those concerns, my own preference would be

my %exceptions = ('' => '', gbr => '');
my $c = $card->country || '';
my $country = exists($exceptions{$c}) ? $exceptions{$c} : uc "[$c]";
[download]

How about

my $country = uc $card->country || ''; # make sure it is UC and define
+d
$country = '' if $country eq 'GBR';    # blat the Brits
[download]

Same caveat about hard coded GBR still applies. It is still brief but I think the logic is clearer so divided. I am aware that will kill $country == 0 but as you are checking for gbr I suspect you do not have numeric countries

update

I missed the addition of [] around the country (sort of proves the danger of too much on one line) so my solution needs another line

or a re-write, I will put my thinking hat back on again !

update 2

with inspiration from perrin

my $def_country = 'GBR'
my $country = uc $card->country || $def_country; # ensure UC & has a v
+alue
$country = $country eq 'GBR' ? '' : "[$country]" # blat brits, add [br
+ackets]
[download]

Cheers,
R.

If $card->country returns 'Gbr', then your code will set $country to '', while the original code will set it to 'Gbr'.

When you have to write code that reads left to right and then right to left you are trying to do two things on the same line that lacks clarity.

If I saw this in production code I would hate forever the person who wrote it.

How about this instead,

my $country = (not $card->country)    ? q{}
  :           $card->country eq 'gbr' ? q{}
  :                                     uc "[$country]";
[download]

It is only a little more verbose but it is clear at presenting the results of any specific situation.

I agree with your approach but would try to avoid using the $card->country call twice.

my $country = $card->country;
$country =
   $country
   ? $country eq q{gbr}
     ? q{}
     : qq{[\U$country]}
   : q{};
[download]

I am unsure of the best way to lay out nested ternaries but have found the above indented multi-line approach, with associated condition, true and false all aligned, quite readable. Others may disagree.

Cheers,

JohnGG

I make this into habit when comes to conditional-chain instead of having a number of blocks only for some single-expressions.

my $some = get_word();
my $when = $some eq 'body'  ? 'has to fight' :
           $some eq 'thing' ? 'has to give'  :
           $some eq 'day'   ? 'they will know the truth' :
           $some eq 'where' ? 'in a avery near place to their mind' :
           $some eq 'time'  ? 'can only tell' :
           'yes, there is always a space for default';
[download]

Compare that to:

my $some = get_word();
my $when;
if ($some eq 'body'); {
    $when = 'has to fight';
} elsif ($some eq 'thing') {
    $when = 'has to give';
} elsif ($some eq 'day') {
    $when = 'they will know the truth';
} elsif ($some eq 'where') {
    $when = 'in a avery near place to their mind';
} elsif ($some eq 'time') {
    $when = 'can only tell';
} else {
    $when = 'yes, there is always a space for default';
}
[download]

Update: Putting it in the given-when construct would be nicer for me. But, until then....

---

Open source softwares? Share and enjoy. Make profit from them if you can. Yet, share and enjoy!

I agree with your approach but would try to avoid using the $card->country call twice

There is basically one thing that needs to be taken into account when using a method call and that is whether or not the method is memory intensive or not.

I like clarity but the overhead savings is something to seriously consider... Thank you for the suggestion.

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

Much clearer. But I agree with halley in that the two cases of the business logic should be handled in separate statements.

_($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                              /\_¯/(q    /
----------------------------  \__(m.====·.(_("always off the crowd"))."·
");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}

• Use block if, not postfix if.
• Reserve postfix if for flow-of-control statements.

Interesting. I've looked at all of the answers, and I don't really find any that are deeply satisfying. I don't pretend to claim that my solution is any better, but...

... I dislike both the if and the ternary. That's confusing. But I also dislike the repeated assignments to $country. Which makes me think that the thing $country contains is not really a country until the end of the second line. Before that, it's only a proto-country that needs, say, a few more fjords.

So to separate the distinction between the not-quite ready, and the final value, I would introduce a separate variable. All tied up in a do block to minimise scope:

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

This way, there is only the single assignment to $country, and when it's done, you have a country.

That was closer to what I came up with but my primary reaction was that if one is spending more than a few seconds on such a very trivial matter (IMHO), then one must not have anything important to work on.

The apparent assumptions that $card->country() is always lower-case and never "0" were somewhat less trivial to me than the not-quite-perfect layout of the code, but I figured such aren't likely problems in the real environment. In any case, my code looked like:

my $displayCountry= do {
    my $c= uc $card->country();
    'GBR' eq $c  ||  '' eq $c  ?  ''  :  "[$c]";
};
[download]

which I still consider only trivially different from most of the other gyrations offered. This of course brings to mind the rude term "turd polishing" but this seem more like "rice polishing", making sure each individual grain is shiny before boiling them all. (:

- tye        

I think it's too much.

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

That, or:

my $country = $card->country;
if ($country and $country ne 'gbr') {
    $country = uc("[$country]");
} else {
    $country = '';
}
[download]

Cast my vote for "too much on one line". The solution I wrote was exactly the same as the first solution above, by ForgotPasswordAgain (lol). I'm honestly surprised that it took this far down the thread for someone to post those simple lines of code. Are we all getting too fancy for our own good...?

---
It's all fine and dandy until someone has to look at the code.

Without wanting to be influenced by the other comments, my opinion is that it's too much, without braces or a comment to make it clear in what order things happen, or why they are happening.

Without the braces I probably have to .. no, I have to look up what the precedence order is, or try it in the debugger. Yes, certainly after developing in Perl for ten years I should probably know that, but I don't. Also, the assignment to $country followed immediately by a re-assignment to $country isn't a style I love.

OK, (after trying it in the debugger) it does do the ternary first and the if statement within that .. that's what I expected.

To answer your question, no, this code is not clear.

Was there a comment explaining the business logic? The left side of my brain suggests ..

  #  Blank country field if Great Britain, otherwise upper
  #  case country name within square brackets. Leave field
  #  empty if country is undefined.
[download]

  #  Since we're mailing from GBR, only display the country
  #  if it's not GBR, and leave a blank country field blank.
[download]

Above all, strive for clarity.

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

<tongue where='in-cheek'>
The proper way to do this would of course be something like:

my $country;
eval {
  my $countryId = Country::Identifier->new(id => $card->country);
  $country = Country::Factory->new(id => $countryId);
  my $cntry_formatter =
    Formatter::Factory->new(action => sub { "[" . shift() . "]" });
  $country->apply(Iterator->new(type => "closure",
                                args => [ $cntry_formatter ]));
}
[download]

Initially, I was worried that there might be a subtle trap involving operator precedence -- does the  if $country apply to the entire ternary operator, or just to its last expression? (I'd have to look that one up and hope the man page description is clear enough to illuminate this particular case.)

But on closer inspection, I see that it doesn't really matter what the scope of  if $country is: in the case where $country is "false" (numeric zero, empty string or undef), nothing will ever happen to change its value; in the case where it isn't false, a value of "gbr" will be set to an empty string, and any other value will have initial and final square brackets added, and have letters (if any) converted to upper-case, regardless how perl actually compiles that line. (I'd want to confirm whether its okay that original values like "GBR" and "Gbr" come out as "[GBR]" instead of empty strings.)

So when properly understood, it seems like the OP snippet is actually "parsimonious" -- but it is still potentially confusing and it takes a while to figure out why it works. I'd rather phrase it like this, to keep the conditions affecting value changes together in a single logical expression:

  $country = ( !$country or $country eq 'gbr' ) ? '' : uc( "[$country]
+" );
[download]

For me, it's not so much a question of "how much logic can fit reasonably on one line", but rather "how much can be left implicit in terms of operator precedence", or "how much effort is needed to figure out the programmer's intent".

Now I'll see what others had to say...

update: Great thread!! It seemed like my alternative was closest to grinder's reply (with tye's grudging assent), except that I'm not objecting to the original notion of assigning a value to $country in each of two consecutive lines.

The complaints about having "left-to-right" and "right-to-left" associations mixed in the single line of code were very apt, and really pinpointed the core of the problem with the OP snippet: bidirectionality in text is always a Bad Thing, to be avoided if at all possible. Always.

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

Since this is very subjective, and the post somewhat of a poll, I'll add my opinion too: differently from most other people here I'm not the very least disturbed by the length of the second line, nor by the fact that as someone said, it "does two things in one statement". Yet I wouldn't do this particular thing in this particular way. Probably I'd pick some of the other alternative mentioned in this thread. Basically I would like the test for country to stay near to that for $country eq 'gbr'. Now that I think of it, I (almost) never do that, but just to mention another WTDI, I could even (slightly) abuse a map for that:

my ($country) = map {$_ and $_ eq 'gbr' ? '' : uc "[$_]"} $card->count
+ry;
[download]

Or, since I prefer to use && to operate on values, perhaps

my ($country) = map $_ && ($_ eq 'gbr' ? '' : uc "[$_]"), $card->count
+ry;
[download]

Update: added parentheses around $country above, as per Roy Johnson's remark. Perhaps when one is not used to abuse, he should not be tempted to do it...

You're using map in scalar context here. I don't think that's what you want.

---

Caution: Contents may have been coded under pressure.

Oh, and what if $card->country is "0"? I'm guessing you would want that translated to "" or "[0]".

Update: I wasn't going to propose an alternative, but ended up doing so as a response to another respondent: Re^2: How Much Is Too Much (on one line of code)?.

Why isn't that just:

my $country = uc("[" . $card->country . "]");
$country = '' if $country eq "[GBR]";
[download]

From the original code, it's clear that $card->country could be "", and, if so, shouldn't get brackets. But you've come closest to how I'd like to think I'd do it (assuming $card->country is always at least defined):

my $country = uc("[" . $card->country . "]");
$country = '' if $country eq "[GBR]" || $country eq "[]";
[download]

(This differs from the original if $card->country starts off as uppercase GBR, or if $card->country is 0 or undef or some overloaded object that acts false, but I'm guessing that's a good thing.)

If $card->country returns '0', your code sets $country to [0], while the original code sets it to 0.

For one thing it would warn if $card->country() is undefined.

---
$world=~s/war/peace/g

Okay, I know it's fun to be a perl hacker and all, but is that one line of code really so much easier/faster/cooler than:

    my $country = $card->country;
    if ($country eq 'gbr') {
        $country = '';
    } else {
        $country = uc "[$country]";
    }
[download]

Now someone will probably fuss that there's some border case where this expanded logic doesn't behave like the cryptic line. (Heck, I'm not even sure they do the same thing in the normal case!) They're probably right.

And that's all the more reason to spell it out. If someone (including the original author) needs to modify that code a year later (or even a week later), are they going to remember all the subtle influences of precedence and truthness that made it so slick the first time around?

So, I guess my answer to the original question is pretty much, "Yes, the code is doing too much on one line." Although it really isn't how much it's doing, just the fact that it's so non-obvious what it intends to do and what it actually does. Can anyone here (other than Ovid) be sure that the code does what the author intended it to do?

There's no point in writing code that makes 96% of perl programmers think really hard, if not pull out a reference or run tests, to figure out exactly what it does in all cases.

Okay, I'll go back to being a stick-in-the-mud (e.g. employed) perl hack now. ;-)

-dave

Here's some untested code that I mostly like:

sub get_country {
    my( $card ) = @_;
    my $tmp= $card->country;
    return "" if ! defined $tmp || $tmp eq "" || $tmp eq 'gbr';
    return uc "[$tmp]"
}

my $country = get_country( $card );
[download]

How do you feel about that code snippet?

Too much :-) Mostly because if the trailing if means I have to re-evaluate a fairly complex LHS after I've read it.

I'd also wonder about the hard-coded 'gbr'. Mostly I'd wonder if some other part of the system should "know" what the default country was. However, if this was the only place it was used I'd probably leave it inlined for the moment.

I'd probably re-write it as something like:

sub display_country {
    return unless my $country = $card->country;
    return '' if $country eq 'gbr';
    return uc "[$country]";
}
[download]

with appropriate tweaks if the coercion of a false country to under/() in scalar/list was inappropriate in context.

I don’t think it need to be re-written. To avoid confusion, I'd like to put two parenthesis and rewrite the code as



my $country = $card->country;    
$country = (( $country eq 'gbr' ) ? '' : uc "[$country]" )if $country;
[download]

rgds
VC
------
If you treat every situation as a life-and-death matter,
you'll die a lot of times.
~Dean Smith~

I'll add the caveat though, that like most issues in communication this relies on an estimate of how the intended audience will read the code, and I can imagine situations where it might be better (or at least okay) to just use the one-liner.

For example, if you needed to do things like this relatively frequently in multiple templates, it would be irritating to have to look at the same block of 4 or 5 lines of code all over the place. If it didn't seem worth dedicating a special routine to the task, then just reusing this one line might seem cleaner.

So, now that the thread has several replies care to fill us in on which side you took?

I'm very much in the "that's too much" camp. I first read the code, thinking through the ternary operator that perhaps it wasn't the best way to write that, and then my thought process came to a screeching halt when I reached the statement modifier. As pointed out by a couple of wise monks, an assignment statement which must be read right to left and then left to right is just confusing.

Of course, it's also trying to pack more than one business rule in the same expression -- something else that is also asking for bugs.

Cheers,
Ovid

New address of my CGI Course.

my $country = do{ local $_ = $card->country and !m[GBR]i ? uc( "[$_]" 
+) : '' };
[download]

or

my $country = do{ 
    local $_ = $card->country and !m[GBR]i 
    ? uc( "[$_]" ) : '' 
};
[download]

I'd write it in the following way:

my $country = $card->country; 
if( $country ) {
    $country = $country eq 'gbr'
               ? ''
               : uc "[$country]";
} # if $country
[download]

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

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

$country and $country = $country eq 'gbr' ? '' : uc "[$country]";
[download]

  my ($country) = map { ( ! $_ or $_ eq 'gbr' ) ? '' : "[$_]" } $card-
+>country;
[download]

well said...
and much much more readable....the best one so far....

my ($country) = map {$_ eq 'gbr'? '': uc "[$_]"}
                grep {$_}
                ($card->country);
[download]

my $country = $card->country;
$country = ($country) ? ( ($country eq 'gbr') ? '' : uc "[$country]" )
+ : $country;
[download]

I've seen lines of code that are a lot longer than that. Hell, I've even written longer than that myself

My personal opinion is that you should put as much on the line as needs to be there. If you can't logically split a statement over multiple lines then put it all on one.