in reply to a simple exercise in readability

Hi apotheon,

I'm a fan of producing a syntax message whenever arguments to a program are expected.  That way, the user never has to guess what the program does, nor what the program expects for input.

Additionally, if you don't check to see that you DID get input, then you'll end up getting errors or "harder to track" unexpected results.

That's why I'd suggest doing an assignment to your command line variables up front, and aborting with a syntax message if the program doesn't get what it wants.  That has the additional benefit of letting you do validity checking on those variables next, as well as simplifying your code later.

Here's how I might "neaten it up" a bit: 

#!/usr/bin/perl -l

# Libraries
use strict;
use warnings;
use Getopt::Std;
use File::Basename;

our ($opt_s);
getopts('s');

# Globals
my $iam = basename $0;
my $syntax = "
    syntax:  $iam  <first number>  <second number>
 
    Your syntax message here.

";

# Command-line
(my $x = shift) or die $syntax;
(my $y = shift) or die $syntax;

# Validity checking
($x =~ /^-?\d+$/) or die "$iam:  value $x not an integer\n";
($y =~ /^-?\d+$/) or die "$iam:  value $y not an integer\n";

# Main program
if ($opt_s) {
    printf "%d\n", ($y - $x + 1) * ($x + $y) / 2;
} else {
    printf "%d\n", ($y - $x) * ($x + $y + 1) / 2;
}

[download]

To my eye, the main program is a lot easier to read now, as a result of assigning to $x and $y.  That's partly because the lines are a lot shorter, so each printf statement fits on its own, single line.

s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/

Replies are listed 'Best First'.
Re^2: a simple exercise in readability
by apotheon (Deacon) on Jan 15, 2007 at 14:33 UTC

    Here's a version incorporating your approach, but in a way that's slightly more idiomatically "mine":

    #!/usr/bin/perl -l
use strict;
use warnings;

use Getopt::Std;

our $VERSION = '1.0';

$Getopt::Std::STANDARD_HELP_VERSION = 'true';

my $help = '
syntax:
  series [-s] <first integer> <second integer>

--help
    prints this help text and exits: invoking the help
    argument causes all other arguments to be discarded
    by this utility

-s
    takes no arguments: changes program behavior so that
    the first number specified is the first number of
    the range, and not the number preceding it (syntax:
    series a+1 n) -- this option must precede all
    numerical arguments to the series utility

description:  This utility takes two integers as
arguments and produces the sum of the range of integers
starting after the first argument and ending with the
second argument.
';

our ($opt_s);
getopts('s');

HELP_MESSAGE() and exit unless $ARGV[1];

foreach (@ARGV) {
  HELP_MESSAGE() and exit unless /^-?\d+$/
}

my $start = shift @ARGV;
my $end = shift @ARGV;

if ($opt_s) {
  print( ($end - $start + 1) * ($start + $end) / 2 )
} else {
  print( ($end - $start) * ($start + $end + 1) / 2 )
}

sub HELP_MESSAGE {
  print $help;
}

    [download]

    I figured I might separate the help text dump for too few numerical arguments from the variable assignments to improve maintainability a tad. I also decided to use the Getopt::Std built in help and version output functionality. Thoughts . . . ?

    edit: johngg pointed out the "unless" bug above, where unless @ARGV should read unless defined @ARGV instead.

    print substr("Just another Perl hacker", 0, -2);
    		 - apotheon
    CopyWrite Chad Perrin

[reply]
[d/l]
[select]
      First of all, apotheon, that's a very nice syntax message.  So many times, when I've run a script or program which gave no clue as to its purpose, I've wished for a syntax message even 1/10th as good as that one, so kudos for writing something clear and descriptive!

      Secondly, instead of: 

      HELP_MESSAGE() and exit unless $ARGV[1];
foreach (@ARGV) {
  HELP_MESSAGE() and exit unless /^-?\d+$/
} 

# ... and later ...
sub HELP_MESSAGE {
  print $help;
}

      [download]

      why not just do the following, which is (I think) more clear, takes less space, and is more precise in the case of invalid input ... 

      die $help unless $ARGV[1];
map { /^-?\d+$/ or die "Invalid input $_\n" } @ARGV;

      [download]
      s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
[reply]
[d/l]
[select]

        Thanks for the compliments on my help message text. I do try to be clear.

        You may have a point, re: your replacement code, with the exception that eliminating the subroutine definition for HELP_MESSAGE() would cause the script to fail to produce the contents of $help when invoked with the --help option. Thus, my two explicit conditional invocations of HELP_MESSAGE() should probably be replaced as you indicated, though I'd keep the subroutine. New code:

        die $help unless $ARGV[1];
map { /^-?\d+$/ or die "Invalid input $_\n" } @ARGV;

# . . . and later . . .
sub HELP_MESSAGE {
  print $help;
}

        [download]

        If I don't keep that subroutine in there, the --help option causes the following output to be produced:

        /home/ren/bin/ies version 1.0 calling Getopt::Std::getopts (version 1.
+05),
running under Perl version 5.8.8.


Usage: series [-OPTIONS [-MORE_OPTIONS]] [--] [PROGRAM_ARG1 ...]


The following single-character options are accepted:
        Boolean (without arguments): -s

Options may be merged together.  -- stops processing of options.

        [download]

        edit: johngg pointed out the "unless" bug above, where unless @ARGV should read unless defined @ARGV instead.

        print substr("Just another Perl hacker", 0, -2);
        		 - apotheon
        CopyWrite Chad Perrin

[reply]
[d/l]
[select]
      You have changed liverpole's

      # Command-line
(my $x = shift) or die $syntax;
(my $y = shift) or die $syntax;

# Validity checking
($x =~ /^-?\d+$/) or die "$iam:  value $x not an integer\n";
($y =~ /^-?\d+$/) or die "$iam:  value $y not an integer\n";

      [download]

      to

      my $start = shift @ARGV;
my $end = shift @ARGV;

      [download]

      It seems to me that you have gained more meaningful variable names but you have lost two forms of error checking, making your script less robust. You no longer detect if the user fails to supply enough arguments and you fail to validate those arguments that are supplied. Probably a backward step.

      Cheers,

      JohnGG

      Update: As apotheon pointed out, I completely missed an element of his code. Please ignore this post.

[reply]
[d/l]
[select]

        You appear to have missed the significance of the following in my version:

        HELP_MESSAGE() and exit unless $ARGV[1];

foreach (@ARGV) {
  HELP_MESSAGE() and exit unless /^-?\d+$/
}

        [download]

        As I stated, I removed the error checking from the variable assignments et cetera. The reason I did so is for maintainability. If you didn't miss that, and find that my approach is deficient, please let me know how it fails to achieve roughly the same results in use.

        edit: More explicitly, I've changed liverpole's

        use File::Basename;
# [unrelated code]
my $iam = basename $0;
# [unrelated code]

# Command-line
(my $x = shift) or die $syntax;
(my $y = shift) or die $syntax;

# Validity checking
($x =~ /^-?\d+$/) or die "$iam:  value $x not an integer\n";
($y =~ /^-?\d+$/) or die "$iam:  value $y not an integer\n";

        [download]

        to . . .

        HELP_MESSAGE() and exit unless $ARGV[1];

foreach (@ARGV) {
  HELP_MESSAGE() and exit unless /^-?\d+$/
}

my $start = shift @ARGV;
my $end = shift @ARGV;

        [download]

        edit: johngg pointed out the "unless" bug above, where unless @ARGV should read unless defined @ARGV instead.

        print substr("Just another Perl hacker", 0, -2);
        		 - apotheon
        CopyWrite Chad Perrin

[reply]
[d/l]
[select]
Re^2: a simple exercise in readability
by kyle (Abbot) on Jan 15, 2007 at 17:38 UTC 
    # Command-line
(my $x = shift) or die $syntax;
(my $y = shift) or die $syntax;

    [download]

    One small problem with this approach is that it does not allow zero as an input. You have to say something like: 

    die $syntax if ! defined (my $x = shift);

    [download]

    ...which is a little more clumsy, since the important part ($x = shift) is way off to the side.

[reply]
[d/l]
[select]
      Yes, good point.  Of course my solution is only intended for cases where one explicitly does NOT want the value of zero as valid input.

      As for the "little more clumsy": 

      die $syntax if ! defined (my $x = shift);

      [download]

      and assuming you needed to allow for zero valued input, I'd be inclined to go with the slightly pithier: 

      defined(my $x = shift) or die $syntax;

      [download]
      s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom