Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

How Much Is Too Much (on one line of code)?

by Ovid (Cardinal)
on Jun 18, 2007 at 12:11 UTC ( #621769=perlquestion: print w/ replies, xml ) Need Help??
Ovid has asked for the wisdom of the Perl Monks concerning the following question:

Had a bit of a disagreement with a colleague about how much "stuff" on a single line of code is appropriate.

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

Note that the second line has both a ternary operator and a statement modifier. One of us argued that this made the code more difficult to read and the other argued that it best reflects the business logic. I won't tell you which of us argued which in order to avoid possibly prejudicing you :)

How do you feel about that code snippet? (Even a comment about the hard-coded 'gbr' would be fine). Is it too much for one line or is it fine? If it's too much, how would you rewrite that to keep the behavior but make it easier to read?

Cheers,
Ovid

New address of my CGI Course.

Comment on How Much Is Too Much (on one line of code)?
Download Code
Re: How Much Is Too Much (on one line of code)?
by Random_Walk (Parson) on Jun 18, 2007 at 12:22 UTC

    How about

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

    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

    $country = "[$country]" if $country;

    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]

    Cheers,
    R.

    Pereant, qui ante nos nostra dixerunt!
      If $card->country returns 'Gbr', then your code will set $country to '', while the original code will set it to 'Gbr'.
Re: How Much Is Too Much (on one line of code)?
by guinex (Sexton) on Jun 18, 2007 at 12:22 UTC
    Too much on one line.

    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.

        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]";

        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]";

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

        Good Day,
            Dean

Re: How Much Is Too Much (on one line of code)?
by Herkum (Parson) on Jun 18, 2007 at 12:31 UTC

    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]";

    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{};

      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';
        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'; }
        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.

Re: How Much Is Too Much (on one line of code)?
by imp (Priest) on Jun 18, 2007 at 12:33 UTC
    I don't mind the ternary, but including the postfix is excessive in my opinion. I'd prefer this:
    my %ignore = ( gbc => 1); my $country = $card->country; if ($country) { $country = $ignore{$country} ? '' : uc($country); } print $country;
Re: How Much Is Too Much (on one line of code)?
by andreas1234567 (Vicar) on Jun 18, 2007 at 12:33 UTC
    Perl Best Practices, Chapter 6: Control Structures
    • Use block if, not postfix if.
    • Reserve postfix if for flow-of-control statements.
    --
    print map{chr}unpack(q{A3}x24,q{074117115116032097110111116104101114032080101114108032104097099107101114})
Re: How Much Is Too Much (on one line of code)?
by perrin (Chancellor) on Jun 18, 2007 at 12:38 UTC
    This is too much because thinking of how to say what it does out loud takes more than a couple of seconds. And yes, 'gbr' belongs elsewhere. And the uc() looks like maybe a display thing that should be in a template somewhere. Even the most naive and verbose rewrite I can think of scans easier for me:
    our $DEFAULT_COUNTRY = 'gbr'; [...] my $country = $card->country; if ($country) { if ($country eq $DEFAULT_COUNTRY) { $country = ''; } else { $country = '[' . uc($country) . ']'; } }

      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.

Re: How Much Is Too Much (on one line of code)?
by Limbic~Region (Chancellor) on Jun 18, 2007 at 12:39 UTC
    Ovid,
    I really don't think the second line is "too much" but it doesn't seem like a good idea either. It seems like the right thing to do would be to overload the country() method and abstract away the problem. Obviously that might not be possible given how country() is used elsewhere in the code.

    An alternative might be:

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

    Cheers - L~R

      Since we're nitpicking a bit, I'll mention that I was wondering when this layout of the ternary operator would appear :)
Re: How Much Is Too Much (on one line of code)?
by naikonta (Curate) on Jun 18, 2007 at 12:49 UTC
    The second line is obviously too much for me, because it does two things in one statement. By one statement, I mean, is terminated by statement terminator, semicolon (;) in this case (explicitly or otherwise). This might help a little,
    $country = $country eq 'gbr' ? '' : uc "[$country]" if $country;
    Two lines, but it's also deceptive, and it's still one statement. So what is "one line of code"? Physically terminated by newline character?

    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]"; }
    But I prefer,
    my $country = ''; my $code = $card->country; $country = "[\U$code]" unless $code eq 'gbr';

    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.


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

Re: How Much Is Too Much (on one line of code)?
by moritz (Cardinal) on Jun 18, 2007 at 13:22 UTC
    Two conditionals on one line, one reading left to right, the other right to left - that's too much for me.
Re: How Much Is Too Much (on one line of code)?
by halley (Prior) on Jun 18, 2007 at 13:44 UTC
    ...argued that it best reflects the business logic...

    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.

    --
    [ e d @ h a l l e y . c c ]

Re: How Much Is Too Much (on one line of code)?
by shmem (Canon) on Jun 18, 2007 at 14:01 UTC
    IMHO there's not too much on that single second line, but it's written in a needlessly confusing way. The postfix if can be avoided with a conditional assignment:
    my $country = $card->country; $country &&= $country eq 'gbr' ? '' : uc "[$country]";

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

    --shmem

    _($_=" "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}
Re: How Much Is Too Much (on one line of code)?
by Zen (Deacon) on Jun 18, 2007 at 15:18 UTC
    The fact that there's an issue at all, that someone has raised concerns, means that it's not clear to your colleagues. If this is whom the intended audience is for, then you should change it for that reason alone (i.e. if your audience can't read it easily, then it's not easily read- period, even if all of perlmonks finds it easy).

    All other opinions are subjective (as to why it's readable or not), but the way I decide what's readable is if someone can formulate a succinct sentence on what a line of code means, from left to right, without having to refer to a reference manual.
Re: How Much Is Too Much (on one line of code)?
by Zaxo (Archbishop) on Jun 18, 2007 at 15:29 UTC

    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]";
    Added: changed uc to a translation escape, just to be different.

    After Compline,
    Zaxo

      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]";
Re: How Much Is Too Much (on one line of code)?
by blazar (Canon) on Jun 18, 2007 at 16:29 UTC
    my $country = $card->country; $country = $country eq 'gbr' ? '' : uc "[$country]" if $country;

    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;

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

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

    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.
Re: How Much Is Too Much (on one line of code)?
by eric256 (Parson) on Jun 18, 2007 at 16:46 UTC

    Why isn't that just:

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

    ___________
    Eric Hodges
      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 "[]";
      (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

Re: How Much Is Too Much (on one line of code)?
by ForgotPasswordAgain (Deacon) on Jun 18, 2007 at 17:01 UTC

    I think it's too much.

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

    That, or:

    my $country = $card->country; if ($country and $country ne 'gbr') { $country = uc("[$country]"); } else { $country = ''; }
      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.
Re: How Much Is Too Much (on one line of code)?
by ysth (Canon) on Jun 18, 2007 at 17:52 UTC
    One thing I don't like is that $country is first used as temporary storage for the result of the function call, then used to mean something almost completely different. Variables are supposed to be variable, but this feels like a case where separate variables would map the business rules better.

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

Re: How Much Is Too Much (on one line of code)?
by talexb (Canon) on Jun 18, 2007 at 18:31 UTC

    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.
    .. but the right side of my brain prevails, and provides a much better comment:
    # Since we're mailing from GBR, only display the country # if it's not GBR, and leave a blank country field blank.

    Above all, strive for clarity.

    Alex / talexb / Toronto

    "Groklaw is the open-source mentality applied to legal research" ~ Linus Torvalds

Re: How Much Is Too Much (on one line of code)?
by grinder (Bishop) on Jun 18, 2007 at 21:18 UTC

    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]" : ''; };

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

    • another intruder with the mooring in the heart of the Perl

      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]"; };

      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        

Re: How Much Is Too Much (Ado About nothing :)?
by BrowserUk (Pope) on Jun 18, 2007 at 23:17 UTC
Re: How Much Is Too Much (on one line of code)?
by sfink (Deacon) on Jun 18, 2007 at 23:30 UTC
    Way too much. It would almost be tolerable if it were:
    my $country = $card->country; $country = $country eq 'gbr' ? '' : uc "[$country]" if $country;
    but maybe that's just me. I always put postfix modifiers on their own line, indented, unless they are for control constructs. I find them much more digestible.

    <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 ])); }
    but I don't have quite enough room left in the margin of this post for the error handling.
    </tongue>
Re: How Much Is Too Much (on one line of code)?
by Argel (Prior) on Jun 18, 2007 at 23:35 UTC
    I think what you meant to ask is if there is too much going on in that statement. I will give a weighted answer of 70% for "just too much" and 30% for "poorly worded". With that said I didn't really like any of the alternatives that much though shmen's (Re: How Much Is Too Much (on one line of code)? ) came close (I consider &&= a bit too obscure so I tend to avoid it).

    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 );
Re: How Much Is Too Much (on one line of code)?
by doom (Deacon) on Jun 19, 2007 at 00:22 UTC
    Like most people, I'm in the "too much" camp, but then I would think so, since I'm not sure I like ternaries at all.

    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.

Re: How Much Is Too Much (on one line of code)?
by graff (Chancellor) on Jun 19, 2007 at 02:53 UTC
    Before reading the other replies on this thread, my first reaction is that I found the snippet potentially confusing -- or possibly confused -- in the sense that its actual outcome is not obviously consistent with its intended outcome. In fact, the thing about the OP snippet that makes it too much work for my taste is figuring out what the intention really is.

    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] +" );

    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.

Re: How Much Is Too Much (on one line of code)?
by adrianh (Chancellor) on Jun 19, 2007 at 04:15 UTC
    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]"; }

    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;
      rgds
      VC
      ------
      If you treat every situation as a life-and-death matter,
      you'll die a lot of times.
      ~Dean Smith~
Re: How Much Is Too Much (on one line of code)?
by perlplexed (Initiate) on Jun 19, 2007 at 05:22 UTC

    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]"; }

    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

Re: How Much Is Too Much (on one line of code)?
by snoopy (Deacon) on Jun 19, 2007 at 05:22 UTC
    I'd consider using map and grep:
    my ($country) = map {$_ eq 'gbr'? '': uc "[$_]"} grep {$_} ($card->country);
Re: How Much Is Too Much (on one line of code)?
by Anonymous Monk on Jun 19, 2007 at 06:17 UTC
    How to add 2+2? Its all the same.
    $country and $country = $country eq 'gbr' ? '' : uc "[$country]";
Re: How Much Is Too Much (on one line of code)?
by Moriarty (Abbot) on Jun 19, 2007 at 06:24 UTC

    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.

Re: How Much Is Too Much (on one line of code)?
by strat (Canon) on Jun 19, 2007 at 09:39 UTC

    I'd write it in the following way:

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

    Best regards,
    perl -e "s>>*F>e=>y)\*martinF)stronat)=>print,print v8.8.8.32.11.32"

Re: How Much Is Too Much (on one line of code)?
by Argel (Prior) on Jun 19, 2007 at 18:09 UTC
    I won't tell you which of us argued which in order to avoid possibly prejudicing you :)

    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.

Re: How Much Is Too Much (on one line of code)?
by yaneurabeya (Novice) on Jun 19, 2007 at 22:14 UTC
    As many have stated, that seems like a bit too much for one line of code. If you moved the overall predicate [ if $country ] to a wrapping conditional it would make a lot more sense and would be more readable than the lines above IMO.
Re: How Much Is Too Much (on one line of code)?
by ysth (Canon) on Jun 19, 2007 at 22:27 UTC
    For those who haven't noticed, I posted a synopsis of Ovid's question to perl-qotw-discuss, where it has had a few answers (some of which are duplicated here).
Re: How Much Is Too Much (on one line of code)?
by wind (Priest) on Jun 20, 2007 at 00:54 UTC
    Like almost everyone else, I agree that this is too much logic in one line. And as has already been stated, the biggest problem is the left to right and right to left logic separation when using the conditional if assignment. To keep the code as close to the original as possible and fix this deficiency, simply use the &&= operator.
    my $country = $card->country; $country &&= $country eq 'gbr' ? '' : uc "[$country]";
    I personally would probably do the following. First declare the initial value. Then perform any business logic to control default countries. Finally assign formatting to the final result.
    my $country = $card->country; $country = '' if $country eq 'gbr'; $country &&= uc "[$country]";
    - Miller
Re: How Much Is Too Much (on one line of code)?
by Anonymous Monk on Jun 20, 2007 at 16:26 UTC
    Great thread...and a very good issue raised...!!!
    Cheers Ovid..
    The problem I think is ...
    1. That is too much on the same line.
    2. And as its already been pointed out that anyone would read the code L to R...
    The line looks like its been written to test someone's analytical ability rather than being easily readable and hence maintanable..
    And I think thats more because of the 2nd problem...the second condition to be evaluated preceeds the first one in L to R order...
    I guess the samething can be written in one line as below:
    my $country = $card->country; $country = ($country) ? ( ($country eq 'gbr') ? '' : uc "[$country]" ) + : $country;

    i think this is more readable....but agree with others that would be better to split this out..!!!!
Re: How Much Is Too Much (on one line of code)?
by simonm (Vicar) on Jun 20, 2007 at 20:11 UTC
    Why use two lines, when one will do? (*grin*)
    my ($country) = map { ( ! $_ or $_ eq 'gbr' ) ? '' : "[$_]" } $card- +>country;
    (And yes, I know this doesn't preserve values of zero or undef, but it seems unlikely that's desirable...)
      well said...
      and much much more readable....the best one so far....

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://621769]
Approved by Tomte
Front-paged by naikonta
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others having an uproarious good time at the Monastery: (7)
As of 2014-12-19 03:27 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    Is guessing a good strategy for surviving in the IT business?





    Results (70 votes), past polls