Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

Re: How would you go about it?

by Aristotle (Chancellor)
on Jul 26, 2004 at 02:18 UTC ( [id://377338]=note: print w/replies, xml ) Need Help??


in reply to How would you go about it?

Check out The Dynamic Duo --or-- Holy Getopt::Long, Pod::UsageMan! and GetOpt::Long usage style. Put together they're how I do all my script writing.

Here are some minor points I'd do differently. (I'll be using my variables instead of your $opts hash, because that affords me the safety of strict. See aforementioned link.)

foreach (@ARGV) { usagedie() if (!-f || !-d); }
I'd prefer to write this as
usagedie() if grep not( -f or -d ), @ARGV;
The following is needlessly repetitive:
find(\&fileop, $opts{'directory'}) unless @ARGV; find(\&fileop, @ARGV) if @ARGV;
It could be written simpler like so:
find( \&fileop, @ARGV ? @ARGV : $opt_directory );
In this part you're checking -f and -d three or four times per entry, and you're repeating yourself in the conditionals. You also print the directory or file names, even if the corresponding mode option was not set, and they were therefor not touched — probably a minor bug.
sub fileop { #set the right permissions based on if a file or a directory #only set permissions of the mode is set chmod oct($opts{'filemode'}), $_ if -f && $opts{'filemode'}; chmod oct($opts{'dirmode'}), $_ if -d && $opts{'dirmode'}; print $File::Find::name . "\n" if $opts{'verbose'} && (-f || -d); }
How about the following? It's longer, but less redundant — there is only one location at which to update or add any single condition.
sub fileop { if( -f and $opt_filemode ) { chmod oct( $opt_filemode ), $_; } elsif( -d and $opt_dirmode ) { chmod oct( $opt_dirmode ), $_; } else { return; # avoid falling thru to print() } print $File::Find::name, "\n" if $opt_verbose; }
Finally, instead of pulling the values through oct every time through the function, I'd do that once at the beginning of the script, as part of input validation that should check whether they're valid modes. Something like
sub check_perm { my ( $mode, $perm ) = @_; if( defined $perm ) { return ( $perm =~ /^[0-7]{3}$/ ) ? oct( $perm ) : die "Malformed $mode mode $perm\n"; } return; } $opt_filemode = check_perm file => $opt_filemode; $opt_dirmode = check_perm directory => $opt_dirmode;

Makeshifts last the longest.

Replies are listed 'Best First'.
Re^2: How would you go about it?
by pcassell (Sexton) on Jul 26, 2004 at 18:41 UTC
    This is great stuff, much appreciated.

    I am glad you talked about the repeated conditionals, since they seem like they would be obnoxious to debug and toubleshoot in a larger script. Thanks.

      NP. :)

      Repetition in general invites all manner of problems (see my other reply further down the thread). Avoid it wherever you can do so with reasonable effort.

      Makeshifts last the longest.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://377338]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others avoiding work at the Monastery: (7)
As of 2024-04-19 14:46 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found