On Tue, Nov 18, 2014 at 05:46:15PM -0500, Harry Putnam wrote:
> #!/usr/bin/perl
>
> use strict;
> use warnings;
>
> if (!@ARGV) {
Alternatively to the @ARGV == 0 suggested by others, there's
always:
unless(@ARGV) { ... }
It's a matter of taste. Nothing really wrong with !@ARGV.
> usage();
> print "At least one directory name is required\n";
> print "Usage tripped: line: <" . __LINE__ . ">\n";
> exit;
> } elsif ($ARGV[0] eq "help") {
> usage();
> print "Usage tripped: line: <" . __LINE__ . ">\n";
> }
>
> my $myscript;
> ($myscript = $0) =~ s/^.*\///;
>
> ## BEGIN GETOPTS SECTION =========================================
> # [HP 12/03/07_20:31:55 Don't use `use vars' anymore ]
> our ($opt_f);
> use Getopt::Std;
> my $optstr = "f";
> getopts($optstr);
It's strange that you're using a Getopt module, but above are
checking for $ARGV[0] eq "help" explicitly. In general, I'd
prefer to see all of the argument parsing happenig at once. And
--help is more standard than merely "help" (there may well be a
file/directory called help).
> my $FORCE; # expects boolean content
If it's meant to be a boolean then I would initialize it as such.
The all-caps seems unnecessary as well.
my $force = 0;
> if($opt_f) { ## force unlink ... no ask
> $FORCE = 'TRUE';
'TRUE' is misleading in the case of a boolean variable because
'FALSE' will still be interpretted as true. You're better off
using actual boolean values in Perl (e.g., 0 and 1).
Additionally, since you are already counting on $opt_f being a
boolean you might as well just copy it instead of branching on
its value. No need for the if statement.
my $force = $opt_f;
> }
I'm not sure how that $opt_f got filled in, but I'm assuming it
is how Getopt::Std works.. In this case I'm not sure I see a need
to copy the value (you could just use $opt_f), but then I'm not
familiar with Getopt::Std. There are alternative Getopt:: modules
that may behave more cleanly. I know that Getopt::Long works
quite nicely and is fully-featured.
> my @trgs = @ARGV;
> @ARGV = ();
> for (@trgs) {
> if (! -d) {
> print "Whoops, no directory <$_> found on system .. skipping\n";
> my $ditchme = shift @trgs; # Get rid of non-starters
> $ditchme = ''; ## zero it out ??
Zero what out? Assigning the empty string to a variable does not
"zero it out". It merely changes what that variable contains.
It's a lexical variable inside of the loop though so it would
lose its content as soon as the loop iterates again. I suspect
that the shift is erroneous as well, but I believe Rob already
showed you a better way to do this so I'll leave it alone.
> }
> }
>
> use File::Find;
> use Cwd 'abs_path';
>
> find(
> sub {
> return unless -f;
> my $file = $_;
> my $abs_path = abs_path($file);
I believe that File::Find::name always contains the absolute path
so I see no need to use abs_path() here..
> if (/~$|\#|\.sw[po]$/) {
I don't know how accurate this definition is so I won't criticise
it, but it does look like a good use for a CPAN module so I
would encourage you to check and see if such a thing has already
been defined better by searching metacpan.org.
> if (! $FORCE) {
> print "\\FOUND: <$abs_path>\n",
> " Shall we unlink <delete> it? <y/n> ";
> my $ans = <STDIN>;
> if ($ans =~ /^y$/) {
> unlink $abs_path or die "Can't unlink <$abs_path>: $!";
> printf "%-60s %s\n","<$abs_path>", "unlinked";
> ## We need to use $_ rather than $File::Find::name because
> ## The latter doesn't work in all situations. And since the
> ## File::Find program is recursing right along with the search
> ## $_ will always work
I'm not sure what this comment is about since there appears to be
no code coming after it, and the code immediately before it isn't
using $_, but I'm not sure the comment is accurate either.
> } else {
> printf "%-60s %s\n", "<$abs_path>", "skipped"
> }
> } else {
> unlink $abs_path or die "Can't unlink <$abs_path>: $!";
> printf "%-60s %s\n", "<$abs_path", "unlinked";
Rather than duplicating this code I'd rather see it restructured
to reuse the same code. This is essentially the "we have
confirmation, delete the file" code. That can either be put into
a separate function, or the want subroutine can be restructured.
While we're reformatting we could also put the definition of what
constitutes an editor backup file into a separate subroutine to
break this one down into as many pieces as possible. Your use of
printf is a little bit weird too. Passing in string literals and
interpolating when you could have those static parts in the
format string.
For example:
sub confirm_unlink {
print "Found: <$abs_path>\n",
"Unlink (delete) it? (Y/n) ";
my $answer = <STDIN>;
return $answer =~ /^y/i;
}
sub is_editor_garbage {
my ($filename) = @_;
return $filename =~ /~$|\#|\.sw[po]$/;
}
sub report {
my ($abs_path, $status) = @_;
# Protip: I'm not sure which is better, interpolating $status
# into the format string, or passing it as an argument. I'm
# sure it's negligible in this case.
printf "<%-60s> $status\n", $abs_path;
}
sub unlink_file {
my ($abs_path) = @_;
unlink $abs_path or die "unlink <$abs_path>: $!";
report($abs_path, 'unlinked');
}
sub wanted {
return unless -f;
my $abs_path = $File::Find::name;
my $filename = $_;
unless(is_editor_garbage($filename)) {
return;
}
unless($force || confirm_unlink($abs_path)) {
report($abs_path, 'skipped');
return;
}
unlink_file($abs_path);
}
> }
> }
> }, @trgs
> );
I'd like to see white-space here. White-space is practially free
and spares your eyes and brain a lot of unneeded strain trying to
figure out where one idea ends and another begins. Use lots of
white-space.
> sub usage{
> print "
> Purpose: Recurse thru list of directories and search out all editor leavings
> (some~ .some.swp some.bak etc), but also includes things like #some# or the
> like.
> Usage:`$myscript dir1 dir2 ... dirN'
> ";
Consider using a HEREDOC in this case. I might also recommend the
use of printf in a large block of text with a single hidden
variable in it because it's easy to miss.
printf <<EOF, $myscript;
Purpose: Recursve through list of directories and search out all editor leavings
(some~ .some.swp some.bak etc.), but also includes things like #some# or the
like.
Usage: %s dir1 dir2 ... dirN
EOF
(Untested)
> }
Hope that helps.
Regards,
--
Brandon McCaig <bamcc...@gmail.com> <bamcc...@castopulence.org>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bamccaig.com/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
signature.asc
Description: Digital signature
