Beefy Boxes and Bandwidth Generously Provided by pair Networks
Keep It Simple, Stupid
 
PerlMonks  

Re: CPCFG_count.pl

by jdporter (Paladin)
on Jun 01, 2007 at 15:27 UTC ( [id://618775]=note: print w/replies, xml ) Need Help??


in reply to Contextualized weighted grammar generator

Yeah, I have a few comments, presented here in decreasing order of importance.

First and foremost, you should be useing strict and warnings.
When you add those, you will get a whole slew of errors, which tell you the next thing you should do:

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.

Avoid "useless use of quotes", aka useless string interpolation. Note that "$foo" evaluates to exactly the same thing as $foo (as long as $foo is already a plain string variable).

I'd recommend using Getopt::Std, rather than hand-rolling a cmdline option parser.

Your die statements for file open errors should include the name of the file, and, ideally, what operation was being attempted and failed.

I recommend using $_ whenever possible, as long as doing so doesn't substantially increase the obfuscation factor. For example, I'd write

while ( <COLL> ) { chomp; @coll = split /:/; $basicColl{$coll[0]} = $coll[1]; }
These are the situations for which $_ was invented!

It's good style always to use a file open mode indicator. It's true that open F, $foo opens the file for reading by default, but, stylistically, open F, "< $foo" (or open F, "<", $foo) 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.)

Some of your parentheses are unnecessary and add to the clutter (at least in some people's opinion). For example,

open(CORP,"<$path") or die ("Error...: $!");
could be written as
open CORP,"<$path" or die "Error...: $!";

You might consider using the "ternary operator". It would significantly clean up certain situations, such as this one:

if($mother) { $lhs="$mothers[$depth]"."_$mothers[$depth-1]"; } else { $lhs="$mothers[$depth]"."_$aunties[$depth]"; }
That could be written as
$lhs = join '_', $mothers[$depth], $mother ? $mothers[$depth-1] : $aunties[$depth];

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 Text::Balanced, Regexp::Common, Text::PromptBalanced, Parse::RecDescent, or perl-lisp.

Lastly, I would recommend thinking of a better title for your post. CPCFG_count.pl 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.

A word spoken in Mind will reach its own level, in the objective world, by its own weight

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others surveying the Monastery: (2)
As of 2024-04-19 20:19 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found