Hi Mike,
you have not replied to my earlier E-mail about trying to explain
the code in MetaCPAN::API. Please do so, or else you'll follow this -
http://www.shlomifish.org/humour/fortunes/show.cgi?id=God_gave_us_two_eyes .
Also - please reply to all recipients - select the right option in GMail.com or
whatever your client is.
On Sat, 10 May 2014 04:01:06 -0500
Mike Dunaway <ekimduna...@gmail.com> wrote:
> So I've just gotten back into Perl and I've written a tarring utility
> for my first application. It seems to work okay, but I'm wondering how
> it could better be written. Any ideas?
>
>
> #!/usr/bin/perl -w
Do not use the -w flag - use "use warnings;" instead:
> use strict;
> use Archive::Tar;
> use Getopt::Std;
Do not use Getopt::Std - use Getopt::Long instead.
> use feature qw(say);
>
> my %opt;
>
> getopts('hcel', \%opt);
>
> help() if $opt{h};
> list() if $opt{l};
> extract() if $opt{e};
> compress() if $opt{c};
These options should be mutually exclusive - maybe do:
if ($opt{h})
{
help();
}
elsif ($opt{l})
{
list();
}
…
>
> sub help {
> say "Easily create and extract tarballs";
> say " -h display this menu";
> say " -c compress file/directory";
> say " | eztar -c <file name you wish to make>
> <file/directory you wish to compress>";
> say " -e extract tarball";
> say " | eztar -e <tarball>";
> say " -l list contents of tarball";
> say " | eztar -l <tarball>";
This should be a here document - https://en.wikipedia.org/wiki/Here_document .
> }
>
> sub list {
> if (defined $ARGV[0]) {
> my $tar = Archive::Tar->new();
> for($tar->list_archive($ARGV[0])) {
Do not use position $ARGV[0] arguments, instead extract out of @ARGV:
http://perl-begin.org/tutorials/bad-elements/#subroutine-arguments
(Note that perl-begin.org is my domain).
Your indentation is wrong, as well.
Finally, do not use $_ for iteration - use a lexical variable instead:
http://perl-begin.org/tutorials/bad-elements/#overuse_dollar_underscore
> say $_;
> exit 0;
> }
> } else {
> die 'No file provided.'
> }
> }
>
> sub extract {
> if (defined $ARGV[0]) {
> my $tar = Archive::Tar->new();
> $tar->read($ARGV[0]);
Indentation is off again.
> $tar->extract();
> } else {
> die 'No file provided.';
> }
> }
>
> sub compress {
> my $file_name = $ARGV[0] or die 'No file name specified.';
> if ($file_name !~ /.*\.tar\.gz$/) {
> die 'File name must end with \'.tar.gz\'';
> }
This regular expression match can be simplified into:
if ($file_name !~ /\.tar\.gz$/)
(without the .* which is unnecessary)
And please consider using \z for end-of-string instead of $.
> my $files = $ARGV[1] or die 'No files specified.';
$files is in plural, but it is a scalar and a single string. What is your
intention? Also see:
http://perl-begin.org/tutorials/bad-elements/#calling-variables-file
> my $tar = Archive::Tar->new();
> $tar->write($file_name);
> $tar->read($file_name);
> $tar->add_files($files);
> $tar->write($file_name, COMPRESS_GZIP);
So far so good.
Regards,
Shlomi Fish
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Free (Creative Commons) Music Downloads, Reviews and more - http://jamendo.com/
I may be a geek, but I’m a true Klingon geek‐warrior! And a true Klingon geek
warrior ALWAYS bottom‐posts.
Please reply to list if it's a mailing list post - http://shlom.in/reply .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
