Update: Like I said, style's contentious. :-)
Thanks to tye for interesting criticism on subroutine
calls... read one of his posts for a
well-thought-out opinion on the topic.
Update: Good call updating to make sure there
are four octets, ybiC. I'm not bothering to update likewise.
Rather than a counter, though, you can just check to
see if @octets10 == 4.
I was going to do something cool for my
100th post, but I got tired of not posting
while I worked on stuff. Oh, well. I never
liked base-10 anyway. ;-)
I have come into the habit of abusing
ybiC's posts in the ChatterBox via
/msg... at ybiC's request. The
conversation often deals with style
as much as anything. These conversations,
however, have outgrown the CB, so I'm
posting this one, and rewriting the
script according to my tastes. Some of
it may be of general interest as well,
but the style opinions are contentious.
Like 80 character lines. Darn it, ybiC,
75 Saves Lives! ;-)
Personally, I don't use & to denote
subroutines except when I'm not
supplying an argument list, and I
want to make my current @_ available
to the sub (check perlsub for
details). So right off, s/&USAGE/USAGE/; I
also don't much care for uppercase subroutine
names, so I changed that too. What the heck.
It is not necessary to declare @octets8 and
$octetpadded as global variables... they'll
serve just fine as lexically scoped variables.
Better not to thwart strict.
my(@octets8,$octetpadded);
This regex is somewhat less than optimal:
$octet10 =~ /\d{1,3}/
It matches octets which contain between
1 and 3 digits, but which may contain other
characters as well. Thus when I enter:
./ipdec2oct.pl 10.m55.000.001
I get an incorrect octal address back:
octal 012.0.0.1
Since you've got warnings enabled, I also
get a message complaining that
Argument "m55" isn't numeric in lt at ./ipdec2oct.pl line 22.
and hopefully that will lead me in the right
direction as a user. But it's a problem.
Now interestingly, if you change your regex
to match octets consisting solely of between
one and three digits...
$octet10 =~ /^\d{1,3}$/
you not only take out the problem of
extraneous characters, you prevent the
possibility of an octet of less than
zero. So you can remove the lines
if ( $octet10 < 0 ) {
&USAGE("\"$address{decimal}\" don't cut it: \"$octet10\" is l
+ess than 0");
}
On to your usage of USAGE. First off, let
me point out the glory of qq(). You're using
double quotes to contain your argument to
&USAGE, because you need variables to interpolate
within the argument... only problem is, you want
double quotes within the argument. This leads
to the frequent use of awkward escape sequences...
&USAGE("\"$address{decimal}\" don't cut it: \"$octet10\" is greater t
+han 255");
Enter the qq operator. qq acts in the same way as
double-quoting a string, and makes use of whatever
delimiters you specify (q does likewise for single
quoting). So this function call could be rewritten
as
&USAGE(qq["$address{decimal}" don't cut it: "$octet10" is greater tha
+n 255]);
or
&USAGE(qq."$address{decimal}" don't cut it: "$octet10" is greater tha
+n 255.);
However, I'm not so sure that USAGE needs an argument.
Why not just print your error message, and then call
USAGE to print the usage message.
if ( $octet10 > 255 ) {
print qq/"$address{decimal}" don't cut it: /,
qq/"$octet10" is greater than 255./;
&USAGE;
}
As it turned out, the way I tweaked things qq() wasn't
even needed; but it's still good to know.
Instead of saying
if ($octet8 > 7) {
$octetpadded = "0$octet8";
}
else {
$octetpadded = $octet8;
}
You could say
$octetpadded = ($octet8 > 7) ? "0$octet8" : $octet8;
which is more readable to me, at least.
For that matter, instead of saying
$octetpadded = ($octet8 > 7) ? "0$octet8" : $octet8;
push @octets8, $octetpadded;
You could say
push @octets8, ($octet8 > 7) ? "0$octet8" : $octet8;
and eliminate the $octetpadded variable entirely.
I don't know whether that's more readable, but
it goes a long way towards explaining why I like
Perl. ;-)
Looking at your foreach loop... a few things.
First, as perhaps you know, foreach and for
are synonyms. I mention this simply because
it's worth mentioning, and I'm changing foreach
to for simply because it's shorter and I prefer
it.
for my $octet10(@octets10) {
Second, as perhaps you know as well, you could
omit the my $octet10 part. If you do, the
particular member referenced will be assigned
to $_ instead. Whether you should do this in
any particular circumstance is a matter of
taste and debate. Here, it could be argued that
the for(each) loop is long enough that using a
more descriptive name enhances clarity. On the
other hand, it could be argued that use of $_
reduces clutter, especially since there are no
nested for(each) loops or other constructions
which would assign to $_. Again, I'm going to
opt for brevity, partially because that's how
I'd do it, and partially because it's interesting
to change things to see a different way.
for (@octets10) {
Instead of saying
my @octets10 = split /\./, $address{decimal};
for (@octets10) {
you have the option to say,
for (split /\./, $address{decimal}) {
You needn't, of course. I just like to because I'm
a bit of a syntactic pervert and I'm into that sort
of thing.
Likewise, I replaced
$address{octal} = join '.', @octets8;
print "\n decimal $address{decimal}\n octal $address{octal}\n\n";
with
print "\n decimal ", $address{decimal},
"\n octal ", join('.',@octets8),
"\n\n";
Of course, that means that there's no longer a
$address{octal}, only an $address{decimal}...
which means a hash is no longer useful. Dang
it! Just after I'd finally gotten you using
hashes. ;-)
To complete the nit-picking, let me point out that
"aint" requires an apostrophe. ;-)
Anyway, with a few more modifications I've not
mentioned, here's my stripped-down version.
Not quite how I'd do it, but that's not the point;
it's still your script at heart. Again,
not all the changes are necessarily improvements.
#!/usr/bin/perl -w
use strict;
my @octets8;
my $ip = shift;
unless (defined $ip) {
print 'Enter *something* for input';
usage();
}
for (split /\./, $ip) {
my $err = "$ip don't cut it: $_";
if ( !/^\d{1,3}$/ ) {
print "$err ain't a number.";
usage();
}
elsif ( $_ > 255 ) {
print "$err is greater than 255.";
usage();
}
else {
my $oct = sprintf "%lo", $_;
push @octets8, ($oct > 7) ? "0$oct" : $oct;
}
}
print "\n decimal ", $ip,
"\n octal ", join('.',@octets8),
"\n\n";
sub usage {
print join "\n ",
"\n Usage: ipdec2oct.pl dottedquad_decimal_ipaddr\n",
$0, "Perl $]", $^O, "\n";
exit;
}
|