perl do's and don'ts

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

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; }` [download] 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 ))); }` [download] 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.	[reply] [d/l] [select]
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	[reply] [d/l] [select]
Re: Re: perl do's and don'ts by Sifmole (Chaplain) on Jun 28, 2001 at 20:47 UTC
`$binary=~/^[01]$/` should probably use a + instead of a `$binary=~/^[01]+$/`	[reply] [d/l] [select]
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";` [download] 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	[reply] [d/l]
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 : not digits `( $binary =~/\D./ )` digits `if ( $binary =~/\d./ ) {` 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;}` [download] into `die ("Not a binary number");` [download] 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` [download] 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>;` [download] 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);` [download] Also, you can combine the chomp and the declaraction/assignment of $binary like this : `chomp (my $binary=<STDIN>);` [download] 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	[reply] [d/l] [select]
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;` [download] so far, so good, `-w` and `use strict` will take you far. `my (@eachnum, $binary, $good, $decimal);` [download] 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;}` [download] 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]/` [download] 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)` [download] 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 ))); }` [download] 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.	[reply] [d/l] [select]
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(); } }` [download] -mr.dunstan	[reply] [d/l]