ryan.rhee has asked for the wisdom of the Perl Monks concerning the following question:

if( ( ($wrongDateFlag == 0) #if the end date is correct && ( (scalar @ARGV >= 3 && $oneDayFlag == -1) #there are three or more arguments in a interv +al model || (scalar @ARGV >= 2 && $oneDayFlag >= 0) #or there are two or more arguments in a one d +ay model ) ) || ($wrongDateFlag == 1 && scalar @ARGV >= 3)#or if the end date is + incorrect #and there are three or more arguments )
My code looks like crap. I have been looking around the internet to figure out what the best practice to clean this up is, but have thus far had no luck. Do you have any suggestions?

Replies are listed 'Best First'.
Re: formatting question
by Fletch (Bishop) on Jun 24, 2008 at 16:35 UTC
    • Don't write it that way to begin with . . . :)
    • perltidy as was mentioned before
    • This is Perl, not C. Explicitly testing against 0 for truth of a boolean (by which I mean solely true/false; if you're (say) encoding state that's a different proposition) is kinda klunky; just use the variable in a boolean sense and lose the == 1 and == 0 stuff
    • Consider refactoring by decomposing the parts, if not into separate subs at least into separate conditional variables (e.g. have my $three_arg_interval_model = scalar @ARGV >= 3 && $oneDayFlag == -1;, and your conditional will read a bit better if( ( $wrongDateFlag and ( $three_arg_interval_model or $two_arg_oneday_model ) ) or $three_arg_wrong_date ) { ... } (and of course you can combine that first and clause into another variable too if you really want))

    Update: And if you're looking to trim verbiage those explicit scalar prefixes are technically extraneous (i.e. just @ARGV >= 3 works just as well), but that's really more a personal preference (not that I don't do it myself from time to time).

    The cake is a lie.
    The cake is a lie.
    The cake is a lie.

Re: formatting question
by jethro (Monsignor) on Jun 24, 2008 at 16:26 UTC
    look for perltidy

    If you also are looking for hints on coding style, the O'Reilly book 'Perl Best Practices' might be something for you.

    My advice on your code sample would be to use 'and' instead of '&&', less errorprone and better to read IMHO. And group the comments together to say what the if clause does altogether instead of commenting on each single term

Re: formatting question
by pc88mxer (Vicar) on Jun 24, 2008 at 16:32 UTC
    perltidy produces essentially the same formatting that you have.

    Another option is:

    my $doit; if ($wrongDateFlag) { $doit = 1 if scalar @ARGV >= 3; } else { # end date is correct if ($oneDayFlag == -1) { $doit = 1 if scalar @ARGV >= 3; } else { $doit = 1 if scalar @ARGV >= 2; } } if ($doit) { ... }
      Minor tweaks...
      my $doit; if ($wrongDateFlag) { $doit = 1 if scalar @ARGV >= 3; } else { # end date is correct $doit = 1 if scalar @ARGV >= (($oneDayFlag == -1) ? 3 : 2 ); } if ($doit) { ... }
        Now I'm getting silly...
        my $doit = scalar @ARGV >= ($wrongDateFlag ? 3 : (($oneDayFlag == -1) + ? 3 : 2 )); if ($doit) { ... }
Re: formatting question
by starbolin (Hermit) on Jun 24, 2008 at 17:18 UTC

    I haven't tested whether this matches your logic, but it should be close, anyway:

    sub body { ... your code ...} SWITCH: { body, last SWITCH if $wrongDateFlag and $#ARGV >= 2; body, last SWITCH if not $wrongDateFlag and $#ARGV >=2 and $oneDay +Flag == -1 ; body, last SWITCH if not $wrongDateFlag and $#ARGV >=1 and $oneDay +Flag >= 0; $felloff == 1; }

    UPDATE:

    I forgot $#ARGV returns one less than scalar @ARGV so I fixed that. Some would probably fault me for using a switch statement where each path calls the same code but I like the resulting look. Cleaner might be.

    sub body { ... your code ...} body if $wrongDateFlag and $#ARGV >= 2 or not $wrongDateFlag and $#ARGV >=2 or not $wrongDateFlag and $#ARGV >=1 and $oneDayFlag >= 0;

    UPDATE:

    removed some vestigial semicolons.


    s//----->\t/;$~="JAPH";s//\r<$~~/;{s|~$~-|-~$~|||s |-$~~|$~~-|||s,<$~~,<~$~,,s,~$~>,$~~>,, $|=1,select$,,$,,$,,1e-1;print;redo}
Re: formatting question
by massa (Hermit) on Jun 24, 2008 at 18:25 UTC
    There are a lot of short ways of writing (and thinking) your code:
    if( @ARGV >= ($wrongDateFlag ? 3 : $oneDayFlag >= 0 ? 2 : 3) ) { }
    or even:
    if( @ARGV >= ($wrongDateFlag || $oneDayFlag == -1 ? 3 : 2) ) { }
Re: formatting question
by InfiniteSilence (Curate) on Jun 24, 2008 at 23:11 UTC
    Since you are mostly working with arguments the best option might be to simply change your strategy altogether and use Getopt::Long instead:

    use Getopt::Long; my ($oneDayFlag, $someDate); my $result = GetOptions ( 'oneday=i'=>\$oneDayFlag, 'somedate=s'=>\$someDate ); ...

    Celebrate Intellectual Diversity