in reply to What makes good Perl code?

You have asked for an opinion about qualities of good code and for discussion, shown an example of your code. I'm sure that you will get a range of opinions!

First, double spacing of the code hinders the readability. Judicious application of white space is one of most important things that you can do to increase legibility of your code. Use a blank line to separate "thought units". What constitutes a "thought unit" is subjective and it takes practice to "develop a feel for it".
BTW: I really like your indenting style - perfect.

I would for example, single space all of the initial print statements and put a blank line before the i=0; statement, or perhaps like this:

print "I will calculate whatever\n", "you specify according to\n", "Ohm's law. What shall I\n", "calculate? Type either\n", "voltage, current, or\n", "resistance.\n"; my $i = 0; ....
Now its easy to see that all those print lines go together as one thing because it is in fact, just one program statement!

The most serious flaw in the code comes here: "while($i == 0)". On a lab assignment, that certainly could be worth a full grade letter deduction (i.e. max possible grade is now a B). This would be true in Java, C, Perl or whatever.

This bad loop design error is important. I have to dig around all inside the body of your loop in order to figure out what ends the loop! The condition that ends the loop, whether it is a "for,"while","until" loop should be part of the loop condition in a very direct way. Also, you have a lot of lines between branches of the if{}elsif{}else{}, I would be thinking about subroutines or otherwise compacting things so that the whole "if" statement fits on a single page. "looks good" is another subjective measurement, but aesthetics are important. Perhaps, something like this:

#!/usr/bin/perl -w use strict; my $line; while( (print "parameter to calculate?:"), ($line=<STDIN>), $line !~ /^\s*q(uit)?\s*$/i) { next if $line =~ /^\s*$/; #blank line => simple re-prompt if ($line =~ /^\s*(resistance|R)\s*$/i) { print "sub to calculate resistance\n"; } elsif ($line =~ /^\s*(voltage|V)\s*$/i) { print "sub to calculate voltage\n"; } elsif ($line =~ /^\s*(current|I)\s*$/i) { print "sub to calculate current\n"; } else { print "illegal entry!\n"; } }
I did use the comma operator. Some Monks have an aversion to it although I think that it is just fine in the right situation. If you don't like it, just add extra statements as needed. My main point is that "while($i==0)" is not an appropriate loop conditional.

Standard expected behavior for a command line program:
- if a mistake is possible, expect that user will do it (maybe unwittingly).
- allow blanks anywhere on input line (not significant anywhere -> begin, end, middle)
- blank line is not an error, just re-prompt
- if there is an error, you have to re-prompt for the user to try again...no getting to question 52 and then blowing up because the user entered "m" for "n"(no)!

The code as presented is not sufficiently tolerant of the user re-doing errors or detecting errors in the first place. This would be another full letter grade deduction.

Well written Perl code would take advantage of the language features. Some folks go crazy with super obscure "tricks". That's not necessary. A good understanding of how Perl regex works and the use of list context on the left hand side of an assignment is not considered obscure. For example, here is one regex statement to parse the amps input....not perfect, but it gets "in the ballpark".

Spaces don't matter. If some text exists past the number, then it must be either "ma" or "mA", "ba" won't work. If $flag_ma is defined, then $amps must also be defined (the division shown will work). If $amps is not defined, then the whole line failed to be valid. Very few statements, but a lot of work is done! As a relative beginner, it might take you some experimentation to figure out how it works, but it should be clear to you what it is supposed to do.

#!/usr/bin/perl -w use strict; my @tests = ('50 ma', '10ma', '2.3', '-.2 ma', '3.', '4.0', '10ba'); foreach my $input (@tests) { my ($amps, $flag_ma)= $input=~ /^\s*([-+]?\d*\.?\d+)\s*(ma|mA)?\s*$ +/; $amps /=1000 if defined $flag_ma; if (defined $amps) { print "$amps amps\n"; } else { print "$input is an illegal entry\n"; } } __END__ 0.05 amps 0.01 amps 2.3 amps -0.0002 amps 3. is an illegal entry 4.0 amps 10ba is an illegal entry
Another thing to consider is that this "top down design", "bottom up implementation" idea isn't so "clean". Maybe because regex is so easy with Perl, you might want to consider a different UI if you know the implementation language is Perl. Maybe something like:
prompt> i = 2.3 prompt> t=3 illegal syntax try again! prompt> r = 3 prompt> v? v=i*r i=2.3 r=3 v=6.9 prompt>v=9 prompt>i? i=v/r r=3 v=9 i=3 prompt>q good bye!
This is a case where I know that it is easy to keep track of the last thing entered for i,v,r (a hash table). I can parse the input syntax easily with one or two regex'es. So I can make the Perl program a zoomy Ohm's law calculator! And probably use less total lines of code than the "dumb" code while at the same time being compact and easy to understand (if you know regex'es). That's great Perl code, do more with less.

So one possible coding of an "one page Ohm's Law Calculator". There are just 3 main cases for the "if": (1) set a variable. i=2.3, (2) print a variable. v=?, and (3) wrong syntax.

#!/usr/bin/perl -w use strict; my %ohmsLaw; #i,v,r print "This is an Ohm's law program (V=I*R).\n", "enter statements like: \n", " Enter I,V,R equation> i=2.3 or 3.2 mA or 3.2 ma\n", " Enter I,V,R equation> R=4\n", " Enter I,V,R equation> v=? **asks for v to be printed**\n", " prints: v=9.2 ** 9.2= (2.3 x 4) **\n", "basically I calculate the value that you ask for\n", " based upon the previous entries for the other two\n", " variables\n", "enter q|Q|quit to quit program\n"; my $line; while( (print "Enter I,V,R equation>"), ($line=<STDIN>), $line !~ /^\s*q(uit)?\s*$/i) { next if $line =~ /^\s*$/; # simple re-prompt on blank line # # set an I,V,R variable... # if (my ($vir_letter, $number, $flag_ma) = $line =~ /^\s*([ivrIVR])\s*\=\s*([-+]?\d*\.?\d+)\s*(ma|mA)?\s +*$/) { if ($vir_letter =~ /I/i) { $ohmsLaw{i} = $number; $ohmsLaw{i} /= 1000 if defined $flag_ma; } $ohmsLaw{v}=$number if $vir_letter =~ /V/i; $ohmsLaw{r}=$number if $vir_letter =~ /R/i; } # # output a calculated I,V,R value # elsif (my ($printValue) = $line =~ /\s*(i|v|r)\s*\=\s*\?\s*$/i) { foreach my $ltr (grep{$_ ne $printValue} keys %ohmsLaw) { print " $ltr=$ohmsLaw{$ltr}\n"; #list other values! } print "v=",$ohmsLaw{v}=$ohmsLaw{i}*$ohmsLaw{r},"\n" if $printValue =~ /V/i; print "i=",$ohmsLaw{i}=$ohmsLaw{v}/$ohmsLaw{r},"\n" if $printValue =~ /I/i; print "r=",$ohmsLaw{r}=$ohmsLaw{v}/$ohmsLaw{i},"\n" if $printValue =~ /R/i; } else { print "illegal syntax!\n"; } }

Replies are listed 'Best First'.
Re^2: What makes good Perl code?
by slinky773 (Sexton) on Aug 16, 2011 at 03:41 UTC
    Did I mention I'm 12? :-D I actually am 12. I have been going through Sam's teach yourself Perl in 24 hours, and instead of going through the code presented in the book, I look at what it teaches me and use it to make stuff that I will actually use, hence making an Ohm's law calculator. Anyways, thanks for the input! I actually don't know regexes all that well, which I know is a shame, considering regexes are one of the most powerful things about Perl, and in many ways the reason why Perl was made in the first place! I guess I should really go through regexes more. Thanks for the input, again.

      Clinton's book is pretty good, but if you're using the third edition it's showing its age. I wrote a book called Modern Perl which covers Perl 5.12. Electronic versions are free. It might be useful for you.

        Well, I also have all these other books from the library here, like the alpaca book Intermediate Perl and Wicked Cool Perl Scripts, but unless I finish up the 24 hour book they won't be of any use to me. I'll probably return them, because at the rate I'm going, it will take about a month rather then 24 hours, ahaha.

      That's cool, I started programming when I was 12 too. Now I've got grandchildren older than you and I'm still learning how to program! I mean, I'm already quite good, but it's not something you ever finish, you just stop one day, usually by dying :)

      Since you're focused on accomplishing tasks and seem to have a basic grasp of Perl, I would recommend The Perl Cookbook because it's a collection of solutions to common tasks that come up over and over again.