Seeking critique of simple file line numbering code.

[David Oswald]

I realize that grep performs some of this same functionality, but I just
wanted to create the tool on my own for the experience. To that end, I'm
also interested in hearing how I might clean it up from every standpoint
(style, robustness, etc.). This script reads infile, prepends a line number
to each line in the file, and outputs it as outfile. Come to think of it, I
probably should have just written the output to stdout to make it easier to
redirect from the shell. But this is just for discussion's sake.

It's pretty basic, and the comments within the file should tell what's going
on.

#!/usr/bin/perl

# Usage:
# linenum [-s] infile [outfile]

# linenum writes outfile with line numbers prepended
# to each line in infile.

# The -s flag causes line numbers in the format of /^d+:/
# to be stripped. Use -s to un-do what was done
# by linenum.


use strict;
use warnings;


# Strip and save command line switches.

my @switch;
foreach ( @ARGV ) {
if ( $_ =~ /^-(.)/ ) {
push @switch, $1;
shift @ARGV;
}
}



# Check for -s switch.

my $remove_numbers = 0;
foreach ( @switch ) {
if ( $_ eq "s" ) {
$remove_numbers = 1;
} else {
print "Invalid option, -$_.\n";
print "Usage: linenum.pl [-s] infile [outfile]\n";
exit;
}
}


# Pull in the filenames.

my ( $infile, $outfile ) = @ARGV;


# If outfile wasn't specified, make up a name.

$outfile = $outfile || $infile.".out";

# Test for file existances.

unless ( -e $infile ) {
print "$infile doesn't exist.\n";
print "Exiting.\n";
exit;
}

if ( -e $outfile ) {
print "$outfile already exists\n";
print "Overwrite? [Y/N]: ";
if (my $decision = <STDIN> !~ /^[yY]/) {
print "Aborting.\n";
exit;
}
}



# Open the files.

open ( INFILE, "<$infile" ) or
die "Unable to open $infile.\n$!";

open ( OUTFILE, ">$outfile" ) or
die "Unable to open $outfile.\n$!";


# Start the counter, and process the files.

my $count = 0;
while ( <INFILE> ) {
# If the remove_numbers flag isn't set,
# increment the counter, prepend number.
unless ( $remove_numbers ) {
$count++;
$_ = "$count: ".$_;
} else {
# Otherwise strip number prefixes.
$_ =~ s/^\d+://;
}
print OUTFILE $_;
}

# Sweep up, turn out the lights, lock the
# door on the way out.
close ( INFILE ) or
die "Unable to close $infile.\n$!";

close ( OUTFILE ) or
die "Unable to close $outfile.\n$!";




--
DJO

[Uri Guttman]

>>>>> "DO" == David Oswald <spamblock@junkmail.com> writes:

DO> use strict;
DO> use warnings;

good start

DO> # Strip and save command line switches.

DO> my @switch;
DO> foreach ( @ARGV ) {
DO> if ( $_ =~ /^-(.)/ ) {

no need for $_ as it is the default bound value for m//

DO> push @switch, $1;
DO> shift @ARGV;
DO> }
DO> }

bad switch handling. either use a standard module or a cpan one. perl
even has simple switch handling with the -s option (see perlrun) but you
would have to deal with a package global that way.

DO> # Check for -s switch.

DO> my $remove_numbers = 0;
DO> foreach ( @switch ) {
DO> if ( $_ eq "s" ) {
DO> $remove_numbers = 1;
DO> } else {
DO> print "Invalid option, -$_.\n";
DO> print "Usage: linenum.pl [-s] infile [outfile]\n";

use a here doc to print multiple lines. that way you see what you get
and you lose the quotes and\n noise.

DO> exit;
DO> }
DO> }

blech. again, a standard arg parsing module is best. the good ones work
with a hash ref so you can keep strict clean. then you have 2 lines, one
to call the module and one to set $remove_numbers from $opts{'s'}.


DO> # Pull in the filenames.

DO> my ( $infile, $outfile ) = @ARGV;


DO> # If outfile wasn't specified, make up a name.

DO> $outfile = $outfile || $infile.".out";

$outfile ||= "$infile.out";

DO> # Test for file existances.

DO> unless ( -e $infile ) {
DO> print "$infile doesn't exist.\n";
DO> print "Exiting.\n";

here doc to the rescue again. multiple prints in a row are fugly and slower.

DO> exit;

why not call die with the error message?

-e $infile or die <<DIE ;
$infile doesn't exist.
Exiting.
DIE


DO> if ( -e $outfile ) {
DO> print "$outfile already exists\n";
DO> print "Overwrite? [Y/N]: ";

you may not see that question as it doesn't end in a newline and won't
get flushed. set $| to make sure that works. see perlvar for info on $|.

DO> if (my $decision = <STDIN> !~ /^[yY]/) {

unless (my $decision = <STDIN> =~ /^y/i ) {

use unless and rarely use !~. it is clearer. and the /i modifier
eliminates the need for the char class


DO> # Open the files.

DO> open ( INFILE, "<$infile" ) or
DO> die "Unable to open $infile.\n$!";

DO> open ( OUTFILE, ">$outfile" ) or
DO> die "Unable to open $outfile.\n$!";


DO> # Start the counter, and process the files.

DO> my $count = 0;
DO> while ( <INFILE> ) {
DO> # If the remove_numbers flag isn't set,
DO> # increment the counter, prepend number.
DO> unless ( $remove_numbers ) {
DO> $count++;
DO> $_ = "$count: ".$_;
DO> } else {
DO> # Otherwise strip number prefixes.
DO> $_ =~ s/^\d+://;

again, no need for $_ there.

DO> }
DO> print OUTFILE $_;

same is true here. print defaults to $_.

DO> }

overall it is ok. some minor style issues and nothing very buggy at
first glance. but then it doesn't do much. :)

uri

--
Uri Guttman ------ [email]uri@stemsystems.com[/email] -------- [url]http://www.stemsystems.com[/url]
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- [url]http://jobs.perl.org[/url]

[Brian Harnish]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, 15 Aug 2003 15:14:08 -0700, David Oswald wrote:

> I realize that grep performs some of this same functionality, but I just
> wanted to create the tool on my own for the experience. To that end, I'm
> also interested in hearing how I might clean it up from every standpoint
> (style, robustness, etc.). This script reads infile, prepends a line number
> to each line in the file, and outputs it as outfile. Come to think of it, I
> probably should have just written the output to stdout to make it easier to
> redirect from the shell. But this is just for discussion's sake.
>
> It's pretty basic, and the comments within the file should tell what's going
> on.

Its pretty good. Instead of $count, you can use $. (its the line number).

Programs like this also become much simpler when ran using stdin and
stdout:
perl -wpe 's/^/$.: /' < infile > outfile
perl -wpe 's/^$.: //' < infile > outfile

or even:
#!/usr/bin/perl -wp
BEGIN {
use strict;
use Getopt::Std;
use vars qw/%opts/;
getopts('s', \%opts);
}
$opts{s} ? s/^$.: // : s/^/$.: /
__END__

- Brian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/PWtdiK/rA3tCpFYRAhi0AJ4mF9vsdzs5XKkB+qmsGW11EoWgPwCeKV7R
MoPjq18u5V7f/CyG0pRGMqI=
=YW+B
-----END PGP SIGNATURE-----

[Michael P. Broida]

David Oswald wrote:

>
> I realize that grep performs some of this same functionality, but I just
> wanted to create the tool on my own for the experience. To that end, I'm
> also interested in hearing how I might clean it up from every standpoint
> (style, robustness, etc.). This script reads infile, prepends a line number
> to each line in the file, and outputs it as outfile. Come to think of it, I
> probably should have just written the output to stdout to make it easier to
> redirect from the shell. But this is just for discussion's sake.
>
> It's pretty basic, and the comments within the file should tell what's going
> on.
>
> #!/usr/bin/perl
>
> # Usage:
> # linenum [-s] infile [outfile]
>
> # linenum writes outfile with line numbers prepended
> # to each line in infile.
>
> # The -s flag causes line numbers in the format of /^d+:/
> # to be stripped. Use -s to un-do what was done
> # by linenum.
>
> use strict;
> use warnings;
>
> # Strip and save command line switches.
>
> my @switch;
> foreach ( @ARGV ) {
> if ( $_ =~ /^-(.)/ ) {
> push @switch, $1;
> shift @ARGV;
> }
> }
>
> # Check for -s switch.
>
> my $remove_numbers = 0;
> foreach ( @switch ) {
> if ( $_ eq "s" ) {
> $remove_numbers = 1;
> } else {
> print "Invalid option, -$_.\n";
> print "Usage: linenum.pl [-s] infile [outfile]\n";
> exit;
> }
> }
>
> # Pull in the filenames.
>
> my ( $infile, $outfile ) = @ARGV;
>
> # If outfile wasn't specified, make up a name.
>
> $outfile = $outfile || $infile.".out";
>
> # Test for file existances.
>
> unless ( -e $infile ) {
> print "$infile doesn't exist.\n";
> print "Exiting.\n";
> exit;
> }
>
> if ( -e $outfile ) {
> print "$outfile already exists\n";
> print "Overwrite? [Y/N]: ";
> if (my $decision = <STDIN> !~ /^[yY]/) {
> print "Aborting.\n";
> exit;
> }
> }
>
> # Open the files.
>
> open ( INFILE, "<$infile" ) or
> die "Unable to open $infile.\n$!";
>
> open ( OUTFILE, ">$outfile" ) or
> die "Unable to open $outfile.\n$!";
>
> # Start the counter, and process the files.
>
> my $count = 0;
> while ( <INFILE> ) {
> # If the remove_numbers flag isn't set,
> # increment the counter, prepend number.
> unless ( $remove_numbers ) {
> $count++;
> $_ = "$count: ".$_;
> } else {
> # Otherwise strip number prefixes.
> $_ =~ s/^\d+://;
> }
> print OUTFILE $_;
> }
>
> # Sweep up, turn out the lights, lock the
> # door on the way out.
> close ( INFILE ) or
> die "Unable to close $infile.\n$!";
>
> close ( OUTFILE ) or
> die "Unable to close $outfile.\n$!";

From another thread, I've recently learned that $.
(dollar dot) contains the line number from your
input file. You don't have to keep your own
$count variable.

(Lots of good learnin' in this group. :)
Mike