Beefy Boxes and Bandwidth Generously Provided by pair Networks
Welcome to the Monastery
 
PerlMonks  

perl do's and don'ts

by Anonymous Monk
on Jun 28, 2001 at 20:27 UTC ( [id://92355]=perlquestion: print w/replies, xml ) Need Help??

Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hello, I'm just trying to figure out where I stand in my learning of perl could someone please point out any sloppy coding, ways I could better do different parts of my code, and anything you can tell me that will help me become more better with my perl coding. =) thanks.
#!/usr/bin/perl -w use strict; my (@eachnum, $binary, $good, $decimal); print("\n\nEnter a binary number:"); chomp($binary=<STDIN>); if ( $binary =~/\D.*/ ) { print("Not a binary number!\n");exit;} if ( $binary =~/\d.*/ ) { @eachnum = split (//, $binary); $good = 1; foreach (@eachnum) { next if /0/ || /1/; print("A binary number contains 1's and 0's!\n"); $good = 0; last; } if ( $good == 1 ) { convb2d(); } } sub convb2d { $decimal = unpack ( "N", pack("B32", substr("0" x 32 . $binary, -32 ))); print ("Decimal: $decimal\n"); exit; } #eNd

Replies are listed 'Best First'.
Re: perl do's and don'ts
by Sifmole (Chaplain) on Jun 28, 2001 at 21:03 UTC
    • You are using a global variable $decimal in sub convb2d. It would be cleaner to pass that value as an argument to the routine, such as
      convb2d($decimal); sub convb2d { my $decimal = unpack ( "N", pack("B32", substr("0" x 32 . $_[0], -32 ))); print ("Decimal: $decimal\n"); exit; }
    • The naming of your routine, sub convb2d would imply to me that it was going to perform a conversion. However, it actually does the conversion, prints the result, and then exits the program. I would have the routine instead simply perform the conversion, and return the result. Imagine future uses -- would this routine be useful if you then wanted to sum up a group of binary numbers in decimal? It wouldn't... but if you did...
      $decimal = convb2d($decimal); sub convb2d { return unpack ( "N", pack("B32", substr("0" x 32 . $_[0], -32 ))); }
      You no longer need the extra variable and can now use this function in many more ways.
    • You don't need '()' around print strings.
    • The previous authors suggestion regarding the regular expression is very good as well.
Re: perl do's and don'ts
by mirod (Canon) on Jun 28, 2001 at 20:40 UTC

    You can simplify the check on the binary number:

    unless( $binary=~/^[01]*$/) die "A binary number contains 1's and 0's! only";

    ^ is the beginning of the string
    [01] is a character class matching 0 and 1 only
    $is the end of the string

      $binary=~/^[01]*$/
      should probably use a + instead of a *
      $binary=~/^[01]+$/
Re: perl do's and don'ts
by CheeseLord (Deacon) on Jun 28, 2001 at 20:59 UTC

    Well, you seem to have the conversion down good (unless I really don't know what I'm doing), but I think you've got far too much code. :) A lot of your testing (as mirod says) could be simplified. Here's what I came up with:

    use strict; sub bin2dec { my $arg = shift; return unpack("N", pack("B32", substr("0" x 32 . $arg, -32 ))); } my ($binary, $decimal); print "Please enter a binary number: "; do { chomp ($binary = <STDIN>) } until (length $binary); die "That's not a binary number, dude...\n" if ($binary =~ /[^01]/); $decimal = bin2dec $binary; print "$binary = $decimal\n";

    My test is similar to mirod's, but it works by checking to see if there's anything besides 1 and 0, instead of if everything is 1 or 0. I also have the program wait until something is actually entered on the line... but these things are subject to personal taste, IMO.

    You'll notice, though, that my bin2dec sub doesn't use any global variables... you'll want to stay away from doing that except in certain situations, as it makes your code less adaptable later on.

    That all being said, though, it's not bad code, it's just difficult to understand with the myriad of unnecessary tests in there. I hope that helps... if you've got any more questions, get an account and /msg me! :)

    His Royal Cheeziness

Re: perl do's and don'ts (boo)
by boo_radley (Parson) on Jun 28, 2001 at 21:31 UTC
    you make 3 checks on the input to see if it contains :
    1. not digits  ( $binary =~/\D.*/ )
    2. digits if ( $binary =~/\d.*/ ) {
    3. binary digits  next if /0/ || /1/;
    cut to the chase. Since the last one contains the crux of your rule, that's the only one you need.
    There's no need to use split on the input, either :
    @eachnum = split (//, $binary); A regex will search through the length of the string, so regexing on each digit has (in this case!) the same effect as regexing on the entire string.
    Also, if you ever plan on forking one script from another, or evaling some code, you should use die rather than exit. die's error code is trappable, exit is not. This turns into
    print("Not a binary number!\n");exit;}
    into
    die ("Not a binary number");
    Also, using a newline in die will surpress the line number in the output. This may be handy if you want to hide that from your users.
    the exit in your sub is redundant.

    In a more general view, all of your variable are global. You might want to look at perlsub to see how to pass parameters between subs.

    however, there are good things, too!
    your code ran with modification!
    your code uses strict!
    and warnings!
    you seem to have a decent grasp of the regex character classes.
    the convb2d sub works well.

    with a little modification, we get :

    #!/usr/bin/perl -w use strict; print "\n\nEnter a binary number : "; my $binary=<STDIN>; chomp ($binary); die ("not binary") if $binary =~/[^01]/; &convb2d ($binary); sub convb2d { my $num2conv = shift; my $decimal = unpack ( "N", pack("B32", substr( $num2conv, -32 ))); print ("Decimal: $decimal\n"); exit; } #eNd
    So with some practice, this can be a 5 line program, and even shorter.

    these 2 line need to remain the same, you can't remove them. Note that we assign a value to $binary at the same time it gets declared.

    print "\n\nEnter a binary number : "; my $binary=<STDIN>;
    remove the newline from $binary and test it. the regex /[^01]/ indicates a negated character class, so anything that's not zero or one.
    chomp ($binary); die ("not binary") if $binary =~/[^01]/; &convb2d ($binary);
    Also, you can combine the chomp and the declaraction/assignment of $binary like this :
    chomp (my $binary=<STDIN>);
    but that's a little complex.

    Overall, good job. It takes some time to let perl sink in, but the flexibility gained is worthwhile. Have fun learning and try not to get frustrated.
    I tried to stay away from things like formatting or use of whitespace; you've probably got a style down that works well for you, and I'd rather help you learn the language than criticize how many returns you have between lines.
    update : typo fix. clarified list

Re: perl do's and don'ts
by Maestro_007 (Hermit) on Jun 28, 2001 at 21:31 UTC
    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:
    convb2d($binary)
    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.

    MM

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

Re: perl do's and don'ts
by mr.dunstan (Monk) on Jun 28, 2001 at 20:57 UTC
    Use more whitespace (blank lines) and indentation to separate each bit of code - you don't have to but it makes things much easier to identify ... this is all - of course - up to the individual person... but if I were to do this I would have formatted it like so ...
    print("\n\nEnter a binary number:"); chomp($binary=<STDIN>); if ( $binary =~/\D.*/ ) { print("Not a binary number!\n"); exit; } if ( $binary =~/\d.*/ ) { @eachnum = split (//, $binary); $good = 1; foreach (@eachnum){ next if /0/ || /1/; print("A binary number contains 1's and 0's!\n"); $good = 0; last; } if ( $good == 1 ) { convb2d(); } }


    -mr.dunstan

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others goofing around in the Monastery: (5)
As of 2024-04-23 17:56 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found