First Perl Program

[DGP]

A few weeks ago I learned that our CAD group was spending way too much
effort to extract point locations from our CAD system. I realized the
problem could be solve by writing a program to do some text file
manipulation.

I decided Perl was the best tool for the job. After reading a begginer
Perl book, I was able to complete the program below within a few
hours. I think this speaks to Perl's ease of use, power, and
flexibility.

It seems to work pretty well, but I wanted to get input to see if it
could be improved.

Thanks Dave

#!C:\Perl\bin\perl.exe
# iges2pt.pl
# Extract point locations from IGES file and write to new text file.
use warnings;
use strict;

my $infile;
my $outfile;
my $line;
my @parts;

# Define Files
print "Enter IGES filename: ";
$infile = <STDIN>;
chomp $infile;
$outfile="points.txt";

# Open Files
open (INF, "<$infile") || die "Cannot open $infile for read.\n";
open (OUTF, ">$outfile") || die "Cannot open $outfile for write.\n";

# Process Data
foreach $line (<INF>) {
# Find lines defining points (Type=116)
if ($line =~ /^116/) {
@parts = split (",",$line);
# IGES format uses 'D' in scientific notation. Replace D with
E.
for (@parts) { s/D/E/ }
# Write out point x,y,z location.
print OUTF "@parts[1..3]\n";
}
}

close INF;
close OUTF;

 

[Tad McClellan]

It seems to work pretty well, but I wanted to get input to see if it
could be improved.

$infile = <STDIN>;
chomp $infile;

You can combine those into a single statement:

foreach $line (<INF>) {

The *entire file* must fit in memory, that is wasteful when you
are only going to process a line at a time anyway.

Read a line at a time instead:

@parts = split (",",$line);

A regex should *look like* a regex:

my @parts = split (/,/, $line);
or
my @parts = split /,/, $line;

 

[Tore Aursand]

#!C:\Perl\bin\perl.exe
# iges2pt.pl
# Extract point locations from IGES file and write to new text file.
use warnings;
use strict;
Excellent!

my $infile;
my $outfile;
my $line;
my @parts;

Generally, don't declare any variables before you use, ie. using them in
the "smallest scope" as possible.

# Define Files
print "Enter IGES filename: ";
$infile = <STDIN>;

How about making it a bit more user-friendly (and making it possible to be
run from another script)?

foreach $line (<INF>) {

Personally, I prefer 'while' here (and I almost always 'chomp' the value,
in addition to almost never using an intermediate variable).

while ( <INF> ) {
chomp;
# ...
}

@parts = split (",",$line);

No need to use double quotes when there's nothing to interpolate.

for (@parts) { s/D/E/ }

IMO, better written the other way around;

s/D/E/ for ( @parts );

Rewritten, your script would look something like this (untested):

#!C:\Perl\bin\perl.exe
# iges2pt.pl
# Extract point locations from IGES file and write to new text file.
#
use warnings;
use strict;

# Define Files
my $infile = ( @ARGV ) ? $ARGV[0] : <STDIN>;
chomp( $infile );
my $outfile= 'points.txt';

# Open Files
open( INF, '<', $infile ) or die "Couldn't open '$infile' for reading; $!\n";
open( OUTF, '>', $outfile ) or die "Couldn't open '$outfile' for writing; $!\n";

# Process Data
while ( <INF> ) {
chomp;
next unless ( /^116/ );

my @parts = split( ',', $_ );
s/D/E/ for ( @parts );

print OUTF "@parts[1..3]\n";
}

# Close files
close( INF );
close( OUTF );

 

[gnari]

A few weeks ago I learned that our CAD group was spending way too much
effort to extract point locations from our CAD system. I realized the
problem could be solve by writing a program to do some text file
manipulation.

I decided Perl was the best tool for the job. After reading a begginer
Perl book, I was able to complete the program below within a few
hours. I think this speaks to Perl's ease of use, power, and
flexibility.

It seems to work pretty well, but I wanted to get input to see if it
could be improved.

[snip script]

if you want a oneliner:

perl -ne"s/D/E/g;print qq($1 $2 $3\n) if /^116.*?,(.*?),(.*?),([^,]*)/"
igesfile >points.txt

notes:
a) as I do not know the IGES format, I do not know if there can be
characters
between the 116 and first comma. thus the .*?
b) hopefully there is no type 1160, but then your code would break too.
c) I am assuming there may be more fields after the 3rd number

gnari

 

[Henry Law]

I decided Perl was the best tool for the job. After reading a begginer
Perl book, I was able to complete the program below within a few
hours. I think this speaks to Perl's ease of use, power, and
flexibility.

Having learnt some Perl myself in the last year, I think it also
speaks to your own abilities. I have learned - taught myself - many
programming languages in the last 30 years and I found Perl harder to
pick up than most others (even including APL ;-). It's not so hard to
write code that more or less does what you want, but to write perlish
code is a knack which only comes after a bit. You've fallen into
some of the non-perlish (but non-fatal) traps as others have pointed
out, but believe me your program is pretty good as a first attempt.

Lurk here .. it's very instructive.

 

[Michele Dondi]

I decided Perl was the best tool for the job. After reading a begginer
Perl book, I was able to complete the program below within a few
hours. I think this speaks to Perl's ease of use, power, and

I see that your post has been duly commented, and it doesn't seem to
me that I'd have anything to add. As an overall cmt, though, let me
tell you that as a "First Perl Program" this is fairly good. You
should really see the kind of crap some people post here...


Michele

 

[J. Gleixner]

#!C:\Perl\bin\perl.exe
# iges2pt.pl
# Extract point locations from IGES file and write to new text file.
use warnings;

[...]
my @parts = split( ',', $_ );
s/D/E/ for ( @parts );

print OUTF "@parts[1..3]\n";

Small improvement. Since only [1..3] seems to be all that's needed in
the end.

my @parts = (split( ',', $_ ))[1..3];
s/D/E/ for ( @parts );
print OUTF "@parts\n";

 

[A. Sinan Unur]

I see that your post has been duly commented, and it doesn't seem to
me that I'd have anything to add. As an overall cmt, though, let me
tell you that as a "First Perl Program" this is fairly good. You
should really see the kind of crap some people post here...

Ditto. It is always good to see someone make the effort.

Sinan

 

[Michele Dondi]

my @parts = (split( ',', $_ ))[1..3];

my @parts = split /,/, $_, 3;


Michele

 

[John W. Krahn]

#!C:\Perl\bin\perl.exe
# iges2pt.pl
# Extract point locations from IGES file and write to new text file.
use warnings;

[...]

my @parts = split( ',', $_ );
s/D/E/ for ( @parts );

print OUTF "@parts[1..3]\n";

Small improvement. Since only [1..3] seems to be all that's needed in
the end.

my @parts = (split( ',', $_ ))[1..3];
s/D/E/ for ( @parts );
print OUTF "@parts\n";

Oh come on, you can do better.

s/D/E/ for my @parts = ( split /,/ )[ 1 .. 3 ];
print OUTF "@parts\n";


John

 

[Michele Dondi]

my @parts = (split( ',', $_ ))[1..3];

my @parts = split /,/, $_, 3;

These aren't equivalent.

Yes, of course they are not! I didn't look carefully and somehow I
assumed the OP just wanted the first three fields. Apologies...


Michele

 

[J. Romano]

I decided Perl was the best tool for the job. After reading a begginer
Perl book, I was able to complete the program below within a few
hours. I think this speaks to Perl's ease of use, power, and
flexibility.

It seems to work pretty well, but I wanted to get input to see if it
could be improved.

Wow! Great program for your first! I wouldn't be able to tell it
was your first program just by looking at it. I especially liked the
way you used "warnings" and "strict". Most beginning Perl programmers
don't use them, and in my experience, many advanced Perl programmers
don't either, which is a shame. "warnigs" and "strict" catch so many
errors that it's sad to see so many programmers who are too lazy to
put them in.

I do want to make three suggestions, however:

The first one is of your line:

@parts = split (",",$line);

The documentation for "split" ("perldoc -f split") says that the
split() function takes a /PATTERN/ (and not a string) for its first
argument. That means that your line would be better written as:

@parts = split(/,/, $line);

Most of the time Perl knows what you mean when you supply a string
instead of a pattern (and in your case it won't make a difference).
However, it's not good to assume that Perl will always know what you
mean. For example, the following two lines are NOT equivalent, even
though they look like they should be:

@tokens = split(" ", "dog horse cat bird");
@tokens = split(/ /, "dog horse cat bird");

My second suggestion to you is to declare the @parts array inside
the:

if ($line =~ /^116/) {
...
}

loop instead of near the top of your program. And the $line variable
should be declared in the foreach declaration. That is, the foreach
declaration should look like:

foreach my $line (<INF>) {

The reason for these changes in declaration is that their position
will now let the code maintainer know that @tokens and $line have no
use outside of the blocks they are declared in. But if they are
declared at the top of the program, then that means they could
possibly be used anywhere in the file.

This is a readability issue, however. If you think that the code
is more readable with the variables declared at the top of the script,
then by all means leave them there. But I'm just letting you know
that if you change them to be declared only inside the blocks (or more
appropriately, the scope) that they are used in, it will be easier for
others to deduce the purpose of what those variables are for.

My last suggestion is one that I learned after years of coding
experience. When you wrote:

# Find lines defining points (Type=116)
if ($line =~ /^116/) {
@parts = split (",",$line);
...
}

it was very good that you included that comment to find type 116
lines. However, I recomment adding a comment line or two showing an
example of what type of line you have found as the first line of the
"if" block. In other words, I recommend making your "if" block look
like this:

# Find lines defining points (Type=116)
if ($line =~ /^116/) {
# Now $line should look something like this:
# 116,blue,$19.95,45.5N,63.0W
@parts = split (",",$line);
...
}

That way, if a maintainer of your code (or you, in a few months) has
to read your code (and not have any input available), it will be
obvious to you/him/her what the input should look like and what the
entries in @parts should look like.

You can also adopt a similar strategy towards the output. Instead of
just:

# Write out point x,y,z location.
print OUTF "@parts[1..3]\n";

You can also add a comment line that shows an example of what could be
printed out, like this:

# Write out point x,y,z location.
# Example output: blue $19.95 45.5N
print OUTF "@parts[1..3]\n";

It helps if the example output line would correspond to the example
input line given above.

An example of the output isn't as important as an example of input,
since a person knowledgeable in Perl should be able to figure it out
relatively easily. But it still helps, espcially if processing the
input is large and complicated.

Of course, adding these comments won't affect the correctness of
your code, but giving an example of the input you are expecting (and
an example of the output you are writing out) is a great help to
anyone who has to read your code (even if it is you), in case the
input format should ever change or someone needs to review your code
to see how something is done (which can definitely happen if your code
works well and is quite useful).

There have been times in the past when I've looked back at my own
code that I wrote a year before and been angry at myself for not
including a sample line of the input I was processing. Conversely,
there have been times when I was happy at myself for including a
sample line or two that helped me understand what exactly the split()
function was operating on.

Overall, I recommend changing your split() function so that it
takes a /PATTERN/ (regular expression) as the first parameter, and
that you include a comment containing an example of a line of input to
process.

Great code, though! (I wish more long-time coders wrote Perl code
like you do!)

-- Jean-Luc

 

[Michele Dondi]

lines. However, I recomment adding a comment line or two showing an
example of what type of line you have found as the first line of the
"if" block. In other words, I recommend making your "if" block look
like this:

# Find lines defining points (Type=116)
if ($line =~ /^116/) {
# Now $line should look something like this:
# 116,blue,$19.95,45.5N,63.0W
@parts = split (",",$line);
...
}

Maybe it's just me, but I *partly* disagree: I think that generally
speaking clobbering code with unnecessary cmts degradates readability
rather than improving it.

In other words well written code must be self-explanatory and only
less than obvious contructs should require explicative cmts.

However in this case what you suggest may be legitimate for the format
of input data may not be obvious, but then in that case I'd rather
just include some info (either cmt or pod) about it and about what the
program is supposed to do with it say at the top of the script itself.

You can also add a comment line that shows an example of what could be
printed out, like this:

# Write out point x,y,z location.
# Example output: blue $19.95 45.5N
print OUTF "@parts[1..3]\n";

Well, in this case IMHO, provided that that kind of info has already
been provided above, such a comment would only be redundant and
disturbing.


Michele