in reply to Re: a simple exercise in readability
in thread a simple exercise in readability

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

Replies are listed 'Best First'.
Re^3: a simple exercise in readability
by liverpole (Monsignor) on Jan 15, 2007 at 15:32 UTC
    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]
        My eyes could be playing me tricks again but I think your

        die $help unless $ARGV[1];

        [download]

        and it's previous incarnations will die erroneously if the user wants to sum from, say, -7 to 0. Perhaps unless defined $ARGV[1]; would be better?

        Cheers,

        JohnGG

[reply]
[d/l]
[select]
Re^3: a simple exercise in readability
by johngg (Canon) on Jan 15, 2007 at 14:51 UTC
    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]
        Sorry, I did miss that. I shouldn't post after two nights of hardly any sleep :(

        Cheers,

        JohnGG

[reply]

In Section Seekers of Perl Wisdom