Don't use -w; 'use warnings;' instead.
# This script allows a quick note to be saved to file
use strict;
my ($len,$string,$file);
Don't declare these so early; wait until you need them. If there's an
error, or the user requests help, you don't need $string or $file at
all. (You don't need $len either, but more about that in a bit.)
$len = scalar @ARGV;
if (($len > 2) || ($len < 1) || ($ARGV[0] eq "--help"))
{
This is a poorly named variable (len implies 'length', usually of
strings, not of arguments). Also, the 'scalar' is unnecessary; the
scalar variable on the left-hand side of the assignment puts it into
scalar context already.
if (@ARGV > 2 or @ARGV < 1 or $ARGV[0] eq '--help')
{
Notice how I used single quotes there. Some people will tell you it's
mandatory; I wouldn't go that far. But as a general principle, I do
think it's helpful to only use double-quotes when you expect to
interpolate a variable in your string.
BTW, this is a bit misleading; it's not the end of the line that
you're stopping at, it's the end of the help text. And most people
around here tend to use all caps for the end-of-heredoc marker.
Also, you're missing a semicolon after the print statement; this isn't
strictly required, since it's the only statement in a block, but what
if you decided to exit afterwards, and put an 'exit 1' at the end of
the block?
This should look like:
print <<EOHELP;
Usage: into [file] string
I recommend using 'single quotes' around string if more than one word.
Will create file with string or append string to file.
If no file specified, string is appended to ~/.into.
end_of_line
}
This is a stylistic thing; I would have put an exit at the end of the
if statement, and not bothered putting the else inside an explicit
else block. To me, this clearly points out that the bits that follow
are the 'normal' operation of the program, and the 'if' statement is
just for exception handling.
Also, I just noticed-- fix your indentation, or lack thereof.
Indentation is a clue to your program's structure. I like three
spaces inside a block, some people like four, others prefer 8. I
don't care what you use, but use *some*, mmmkay?
Remember, whitespace is NOT endangered! Use it!
if ($len == 1)
{
$file = "$ENV{'HOME'}/.into";
$string = $ARGV[0];
}
else
{
$file = $ARGV[0];
$string = $ARGV[1];
}
Indent!
At this point, I've forgotten what $len even means; I have to go look
for it. This wouldn't normally happen for such a small program, but
I've written quite a few comments already, so my brain got full. If
you swapped your command-line around so that it was
into string [file]
Your code could look like:
my ($string, $filename) = @ARGV;
$filename = "$ENV{HOME}/.into" unless $filename;
Even as it is, you could write this whole silly parsing block as
my ($filename, $string) = @ARGV;
if (!defined $string) { # only one argument passed in
$string = $filename;
$filename = "$ENV{HOME}/.into";
}
Always always always ALWAYS check the return from an open()! I cannot
emphasize this strongly enough. You probably ought to check on
close() as well-- in fact, if you're going to be anal, you should
check the results of every function that references a system call.
But you wouldn't believe the sheer number of problems we have
uncovered in this newsgroup by virtue of people simply rewriting that
as
open OUT, ">>$file" or die "Couldn't open $file: $!";
Just Do It(tm).
print OUT "$string\n";
close OUT;
You can also do this with lexical filehandles, and the three-arg form
of open() (which I think it the cat's pyjamas):
open my $file, '>>', $filename or die "Couldn't open $filename: $!";
print $file $string,"\n";
close $file or die "Error closing $filename: $!";
Fine, but unnecessary.
-=Eric
