Beefy Boxes and Bandwidth Generously Provided by pair Networks
XP is just a number

Re: perl do's and don'ts

by Maestro_007 (Hermit)
on Jun 28, 2001 at 21:31 UTC ( #92389=note: print w/replies, xml ) Need Help??

in reply to perl do's and don'ts

I'm going to stick to the glaring stuff. There's more, but beyond this what I can come up with goes toward the "readability vs. performance" issues, rather than empirically good vs. bad code. Also, comments would be nice.
#!/usr/bin/perl -w use strict;

so far, so good, -w and use strict will take you far.

my (@eachnum, $binary, $good, $decimal);

I would lose most of these (see below) and put $binary in the if statement.

print("\n\nEnter a binary number:"); chomp(my $binary=<STDIN>); if ( $binary =~/\D.*/ ) { print("Not a binary number!\n");exit;}
hmm... /\D.*/. I assume you want to check for the presence of non-digits in the entry. Remember that .* checks for anything, so you're checking for "a non-digit character followed by 0 or more of anything". Besides the obvious warning about .* (see Death to Dot Star!), you just need /\D*/

Furthermore, my impression is that you want to make sure the entry contains nothing but 1s and 0s, right? With a character class, you can take care of that with [^01], and narrow this whole thing down to a single if. I'd also change the print "errmsg"; exit; to a simple die, or maybe a warn and a retry attempt:

die "Not a binary number" if $binary =~ /[^01]/
Then you can go straight to the subroutine convb2d(), and I would pass $binary to it, rather than treating the value as a global:
In the subroutine, either use shift where you now have $binary, or do my $binary = shift; at the top of the subroutine. here's the complete code (tested):
#!/usr/bin/perl -w use strict; #get the input print("\n\nEnter a binary number:"); chomp(my $binary=<STDIN>); #make sure the number is only 1s and 0s die "Not a binary number: $!" if $binary =~ /[^01]/ #get the value and print it to the screen my $decimal = convb2d($binary); print "Decimal: $decimal\n"; #do the conversion sub convb2d { my $binary = shift; return unpack ( "N", pack("B32", substr("0" x 32 . $binary, -32 ))); }
There's more you could do, and I'm sure you could get the whole thing down to 2 lines, but this covers the major areas. Mainly, if you have to jump through that many hoops to do input validation on such a simple problem, remember that since TMTOWTDI, there's a high probability that one of those ways is much simpler.


UPDATE: removed $! from die at chromatic's suggestion. It was completely unnecessary, as there is no condition under which it will return usable information.

Log In?

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

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (6)
As of 2023-02-01 16:49 GMT
Find Nodes?
    Voting Booth?
    I prefer not to run the latest version of Perl because:

    Results (11 votes). Check out past polls.