Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine

Improve my coding

by cheech (Beadle)
on Jun 12, 2009 at 22:23 UTC ( #771126=perlquestion: print w/replies, xml ) Need Help??
cheech has asked for the wisdom of the Perl Monks concerning the following question:

I'm training myself in Perl and have been coding some basic programs that my professor gave me. The one below works just fine, but I thought I'd gain some insight to more efficient coding if I posted it for the community to critique. Any and all suggestions or comments are greatly appreciated! #loop over drop radius from 1 micron to 100 microns in steps of 2 micr +ons #use each radius to compute cross section #if radius <20 microns, use (pi)*(r^2)*a*(1-exp(-c*r)) #if radius >= 20 microns, use 2*pi*(r^2) #VARIABLE DECLARATION #-------------------- my ($r, $rad, $twopi, $sig, $pi, $a0, $c0); my (@radii, @rads, @sigs); #-------------------- use strict; @radii = (1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23, 25, 27, 29, 31, 3 +3, 35, 37, 39, 41, 43, 45, 47, 49, 51, 53, 55, 57, 59, 61, 63, 65, 67 +, 69, 71, 73, 75, 77, 79, 81, 83, 85, 87, 89, 91, 93, 95, 97, 99); @rads = (); @sigs = (); $pi = 3.14159; $twopi = (2)*(3.14159); $a0 = 1.18; $c0 = (0.28e6); foreach $r (@radii) { $rad = (1e-6)*$r; push (@rads, $rad); if ($r>20) { $sig = $twopi*($rad**2); push (@sigs, $sig); } else { $sig = $pi*($rad**2)*$a0*(1-exp((-$c0)*$rad)); push (@sigs, $sig); } } print "@rads \n"; print "@sigs \n";

Replies are listed 'Best First'.
Re: Improve my coding
by chromatic (Archbishop) on Jun 12, 2009 at 23:03 UTC
    my ($r, $rad, $twopi, $sig, $pi, $a0, $c0); my (@radii, @rads, @sigs);

    This is usually a bad idea; it dumps all of your variables (some of which can be lexical) into file-scoped lexicals. No one can look at a variable declaration and see its intended scope now. (For example, $r can easily be lexical to the for loop. $rad and $sig can be lexical within the loop.)

    @rads = (); @sigs = ();

    That code doesn't do anything. Freshly-declared arrays are already empty.

Re: Improve my coding
by JavaFan (Canon) on Jun 12, 2009 at 22:32 UTC
    Suppose you had to loop from 1 to 1000000 in steps of square root of 2. Would you have precalculated all the intermediate points and put them in an array? I certainly wouldn't. I'd write your loop as:
    for (my $r = 1; $r <= 100; $r += 2) { ... }
    I'd also declare the variables I only used in the body of the loop in the body, and I wouldn't make them global. Furthermore, if I were to make constants pi and 2pi, I'd define the latter as a function of the first:
    $twopi = 2 * $pi;
    If you then use a better approximation of pi, you'd only have to update the program once.
Re: Improve my coding
by repellent (Priest) on Jun 13, 2009 at 00:00 UTC
    To reiterate chromatic, you should generally "declare" your my variables as late as possible so that they are scoped in the appropriate BLOCKs.

    Let's take $sig for example. When you declared
    my ($r, $rad, $twopi, $sig, $pi, $a0, $c0);

    what you're saying is $sig is available for the rest of the entire program (file-scoped). That works, but it's good practice to only scope $sig where it is used.

    Hence, remove my (..., $sig, ...); above and rewrite your if-statement like:
    if ($r>20) { my $sig = $twopi*($rad**2); push (@sigs, $sig); } else { my $sig = $pi*($rad**2)*$a0*(1-exp((-$c0)*$rad)); push (@sigs, $sig); }

    Now $sig is lexically scoped to either of those two BLOCKs. Try using $sig outside those blocks and you'll trigger a warning. This is a good thing.

    Why? This approach leads to cleaner code in the long run:
    • Less confusion, easier debugging.
    • It allows you to structure your thoughts better when variables are "compartmentalized".
    • Plus, it's way easier to declare when-you-need-it rather than maintaining a huge list at the very beginning of the program.
Re: Improve my coding
by demerphq (Chancellor) on Jun 13, 2009 at 11:17 UTC

    Well your style will make it difficult to work with others. Have a look at the common styles of coding. If your style isnt relatively close to them then you can a) expect other coders to whine about your code and b) constantly be working on code which doesn't match your style. Which means a combination of frustrating your co-workers and annoying yourself.

    Coding style isnt like choosing a pair of shoes. Its about writing code that you and your colleagues can work on and understand easily.


Re: Improve my coding
by Anonymous Monk on Jun 12, 2009 at 23:48 UTC
    Your $sig calculation is conditional, but you push the same variable with either condition. Might as well push it once, after the if-else:
    $sig = ($r>20) ? $twopi*($rad**2) : $pi*($rad**2)*$a0*(1-exp((-$c0)*$rad)); push (@rads, $rad); push (@sigs, $sig);
Re: Improve my coding
by jwkrahn (Monsignor) on Jun 13, 2009 at 03:35 UTC
    #!/usr/bin/perl use warnings; use strict; #loop over drop radius from 1 micron to 100 microns in steps of 2 micr +ons #use each radius to compute cross section #if radius <20 microns, use (pi)*(r^2)*a*(1-exp(-c*r)) #if radius >= 20 microns, use 2*pi*(r^2) my @radii = grep $_ & 1, 1 .. 100; my $pi = atan2 0, -1; my $twopi = 2 * $pi; my $a0 = 1.18; my $c0 = 0.28e6; my ( @rads, @sigs ); foreach my $r ( @radii ) { push @rads, 1e-6 * $r; # push @sigs, $r > 20 ? $twopi * $rad ** 2 : $pi * $rad ** 2 * $a0 * + ( 1 - exp -$c0 * $rad ); # **Update: Oops, changed $rad to $rads[-1] push @sigs, $r > 20 ? $twopi * $rads[-1] ** 2 : $pi * $rads[-1] ** + 2 * $a0 * ( 1 - exp -$c0 * $rads[-1] ); } print "@rads\n"; print "@sigs\n";
Re: Improve my coding
by jbt (Chaplain) on Jun 13, 2009 at 15:02 UTC
    The output of your program is hard to read. Consider using the Perl Format mechanism. See perldoc perlform
      maybe you could consider putting the radii and the cross sections in the same array as they seem to belong together.
      #!/usr/bin/perl -w
      # loop over drop radius from 1 micron to 100 microns in steps of 2 microns
      # use each radius ($r) to compute cross section
      # if r < 20 microns,  use (pi)*(r^2)*a*(1-exp(-c*r))
      # if r >= 20 microns, use 2*pi*(r^2)
      use strict;
      use Data::Dumper;
      my @cross_sections = ();
      my ($pi, $a, $c) = (atan2(0, -1), 1.18, 0.28e6);
      for (my $i = 1 ; $i < 100; $i += 2) {
          my $r = $i / 1000000;
          if ($i < 20) {
              push @cross_sections, [$r, $pi * $r**2 * $a * (1 - exp(-$c * $r))];
          } else {
              push @cross_sections,[$r, 2 * $pi * $r**2];
      print Dumper(\@cross_sections);
Re: Improve my coding
by Zen (Deacon) on Jun 15, 2009 at 15:35 UTC
    Going to go against the popular vote on the variable declarations at the top. Old style rules in other languages put them at the top so as to make the question "Where are these from" easier. Easy to find and easy to change; burying them in conditionals stinks IMO.

      The compelling reason for putting variables in the smallest scope possible is that it greatly reduces the likely hood of "action at a distance" style errors that are the bane of using global variables.

      Another reason is that good coding style dictates that you declare the variable the first time it is used and initialize it at the same time. It's good coding style because it makes it clear where the variable is first used and how it gets its initial value.

      True laziness is hard work

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://771126]
Approved by Perlbotics
Front-paged by toolic
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others wandering the Monastery: (7)
As of 2018-04-24 09:53 GMT
Find Nodes?
    Voting Booth?