note
jdporter
<p>
Yeah, I have a few comments, presented here in decreasing order of importance.
</p><p>
First and foremost, you should be [doc://use]ing [doc://strict]
and [doc://warnings].<br/>
When you add those, you will get a whole slew of errors, which tell you
the next thing you should do:
</p><p>
Use lexical variables to the extent possible. And secondarily to that,
declare lexical variables in the nearest enclosing scope as possible.
In most cases that will be the innermost scope in which the variable
is first used, but occasionally you need it to survive that scope, so
moving the declaration out the necessary number of levels is appropriate.
</p>
<p>
Avoid "useless use of quotes", <i>aka</i> useless string interpolation.
Note that <c>"$foo"</c> evaluates to exactly the same thing as <c>$foo</c>
(as long as $foo is already a plain string variable).
</p>
<p>
I'd recommend using [mod://Getopt::Std], rather than hand-rolling a cmdline
option parser.
</p>
<p>
Your [doc://die] statements for file open errors should include the name of the file, and, ideally, what operation was being attempted and failed.
</p>
<p>
I recommend using <c>$_</c> whenever possible, as long as doing so doesn't substantially increase the obfuscation factor. For example, I'd write
<code>
while ( <COLL> )
{
chomp;
@coll = split /:/;
$basicColl{$coll[0]} = $coll[1];
}
</code>
These are the situations for which <c>$_</c> was invented!
</p>
<p>
It's good style always to use a file open mode indicator.
It's true that <c>open F, $foo</c> opens the file for reading by default,
but, stylistically, <c>open F, "< $foo"</c> (or <c>open F, "<", $foo</c>)
is better. (It is said that that last form, the "three-argument open", is
best of all, as it prevents a certain class of bugs.)
</p>
<p>
Some of your parentheses are unnecessary and add to the clutter
(at least in some people's opinion). For example,
<code>
open(CORP,"<$path") or die ("Error...: $!");
</code>
could be written as
<code>
open CORP,"<$path" or die "Error...: $!";
</code>
</p>
<p>
You might consider using the "[doc://perlop#Conditional-Operator-operator,-conditional-operator,-ternary-ternary-?:|ternary operator]".
It would significantly clean up certain situations, such as this one:
<code>
if($mother)
{
$lhs="$mothers[$depth]"."_$mothers[$depth-1]";
}
else
{
$lhs="$mothers[$depth]"."_$aunties[$depth]";
}
</code>
That could be written as
<code>
$lhs = join '_',
$mothers[$depth],
$mother ? $mothers[$depth-1] : $aunties[$depth];
</code>
</p>
<p>
I'm not sure, but it looks like you're just parsing LISP-like structures,
with balanced parentheses. If so, you're in serious wheel reinvention
territory. It's fine for learning, but for production you might want to
consider using a module such as [mod://Text::Balanced], [mod://Regexp::Common], [mod://Text::PromptBalanced], [mod://Parse::RecDescent], or [dist://perl-lisp].
</p>
<p>
Lastly, I would recommend thinking of a better title for your post.
<c>CPCFG_count.pl</c> may work for you as the script's filename,
but PerlMonks titles should convey some information.
It helps to think of the title as the place to put key words.
Thanks.
</p>
<div class="pmsig"><div class="pmsig-170442">
<small><i>A word spoken in Mind will reach its own level, in the objective world, by its own wei[http://www.sacred-texts.com/eso/som/som19.htm|g]ht</i></small>
</div></div>
618629
618629