In article <36797BF6.B8840280@psu.edu>, Brett Borger <bxb121@psu.edu> wrote:
>The definitions file is in the format:
>Name: Item1
>Value: Bob
>Special: Frank
>Name: Item2
>Special: None
>my(%list)={}; #Do I need to do this?
A. No. It's empty when you get it.
B. It's wrong anyway. {} is a reference to a hash, which is a single scalar value. But when you assign to a hash, you have to specify a list of pairs, like this:
%Wife = ( Ralph => Alice, Ed => Trixie, Nat => Jenine );
Note parentheses, not braces. See
http://www.plover.com/~mjd/perl/FAQs/Reference.html
for a short explanation of reference syntax.
>I get three "Odd number of hash elements" warnings (which I don't
>understand)
%hash = ( Ralph => Alice );
makes sense, because hash items always come in key-value pairs.
%hash = ( );
makes the hash empty.
%hash = ( Bonzo );
generates an error: `Odd number of hash elements', because there should have been an even number, because hash items always come in key-value pairs.
%hash = Bonzo;
is exactly the same; the parentheses weren't adding any meaning. In this case, Perl' just ignores the extra item and makes the hash empty.
%hash = {};
is similar: {} is a scalar value: it's a *reference* to an anonymous hash. So you're trying to assign a single item to a hash, and hashes always want items in pairs.
By the way, the `perldiag' manual page explains this in detail; it has an explanation of every error message.
>open(DATA,$filename);
No! No! Bad programmer! No biscuit.
open(DATA,$filename)
or die "Couldn't open `$filename' for reading: $!; aborting";
But you know, it seems like a shame to write a program that only works on one file. Someday it might be useful to be able to say
process file1 file2 file3
and have it read in all three files, or to be able to say
someprogram | process -
to have it process the output of `someprogram'. There's a really easy way to get that: Instead of opening the file yourself, just say
while (defined($line = <>)) {
The <> means to read from the files named on the command line, or from the standard input if there are no files named on the command ilne. On the command line, `-' means to read the standard input anyway.
Now you can play a sly trick here:
unless (@ARGV) { @ARGV = ($filename) }
If there are no command line arguments, you pretend that the user specified the default file, and you go ahead and process it with <> anyway.
>READIN: while(defined($line=<DATA>)){
Labels should be named according to the thing they control, like `LINE' or `SECTION', because that way things like this make sense:
next LINE;
last SECTION;
Now, the main loop of your program is broken in a very interesting way. You accumulate a bunch of stuff in variables, ($thisitem,
$thisname, $thiscontent, %list) and each time you see a new thing, you install the current thing into the main data structure and then save the new thing w into the variables here the current thing was.
This is a bad design, because you always have some data left over in these variables that hasn't made it into the data structure yet. You have a bunch of extra variables, and they have data in them that is left over after you've read the input. A special part of the program will have to gather up the missed data from these variables at the end. (I think you forgot to do this anyway.)
In a well-designed program, the structure of the program will
correspond to the structure of the input. One good principle to follow is that each thing should be installed into its final place as soon as possible; that helps avoid errors in which you've forgotten to something.
In my version of the program, we create a new item as soon as we see the `Name' that introduces it. It's incomplete, because we haven't read its properties yet, but that's all right. We will build it up as we go, and if it turns out to be erroneous, it is easy to have the program remove it again.
This is probably the most important part of this article, by the way. All the other stuff is trivia, but this issue is central to
programming.
> push @items, \%list unless $notready;
> $gear{$thisname}=\%list unless $notready;
> $notready=0;
I think it would have been easier to understand if you had replaced `notready' with `ready':
> push @items, \%list if $ready;
> $gear{$thisname}=\%list if $ready;
> $ready=1;
Then you don't have to initialize `$notready'. But it would have been even simpler to ditch `$notready' entirely:
> push @items, \%list if defined $thisitem;
> $gear{$thisname}=\%list if defined $thisitem;
or some such. Also, it's silly to explicitly construct @items. If you have %gear, you can get @items for free:
@items = values %gear;
If the idea here was to remember the order in which the items
appeared, there are several solutions that are better than the one you chose. The simplest is to make a list of the names:
push @items, $thisname;
Also, you're pushing in a reference to a hash, and then a few lines later you (try to) empty out the hash; that means that the reference you just put into %gear now refers to an empty hash. Ooops. To evade this, you need to copy the hash and install a reference to the copy into %gear:
$gear{$thisname} = { %list };
But see the way I did it, below.
> ($thisitem,$thiscontent)=split(/:/,$line);
Here you just misunderstood the Perl `split' operator. If $line had
Hamlet: Alas, poor Yorick! I knew him, Horatio: a fellow of infinite jest
Then `$thisitem' gets `Hamlet', which is what you want, and
`$thiscontent' gets ` Alas, poor Yorick! I knew him, Horatio'. You may be unhappy about the space at the beginning, but you will probably be much more unhappy to lose `a fellow of infinite jest.' Poor Yorick indeed! You should use:
($thisitem,$thiscontent) = split(/:\s*/, $line, 2);
The `2' on the end tells it to only split into 2 items, and not to bother splitting up the second one.
>for ($i=0;$i<=$#items;$i++){
Any time you write this in Perl, back up and see if you haven't maybe made a mistake. It's almost always better to say
foreach $item (@items) {
Because then this nastiness:
> foreach $key (keys(%{$items[$i]})){
> print "-$key: ${$items[$i]}{$key}\n";
becomes much less nasty:
> foreach $key (keys(%$item)){
> print "-$key: $$item{$key}\n";
Actually you can do even better:
> while (($key => $value) = keys %$item) {
> print "-$key: $value\n";
Now it's actually readable!
Of course, if you use `foreach $item (@items)' you don't get the index number that you need for
> print "Item: $i\n";
If you really need that, here's how to write it. Instead of this:
>for ($i=0;$i<=$#items;$i++){
Say this:
>for ($i=0; $i < @items; $i++) {
Note the white space that breaks up the line into its logical
components. If in doubt, always add a space after a comma or a
semicolon.
Then instead of this:
> print "Item: $i\n";
> foreach $key (keys(%{$items[$i]})){
> print "-$key: ${$items[$i]}{$key}\n";
Use this:
> print "Item: $i\n";
> $item = $items[$i];
> foreach $key (keys %$item) {
> print "-$key: $$item{$key}\n";
Or:
> print "Item: $i\n";
> $item = $items[$i];
> while (($key, $value) = each %$item) {
> print "-$key: $value\n";
Perl has too much punctuation, so it's always a good idea to use as little as possible.
>And that is all. I'd fight my way through it, but I'm not sure which
>end I screwed up, the storage or retreival.
Storage. Retrieval was basically sound, if somewhat turgid. Although I notice that you never emitted the names. Was that on purpose?
>Any help is appreciated,
#!/usr/bin/perl -w
use strict;
my $cur_name = undef;
my %property; # This is where we store the input
unless (@ARGV) { @ARGV = ('/the/default/data/file') }
# Read in the input files
LINE: while (<>) {
s/#.*//; # Trim comments
/\S/ or next LINE; # Skip entirely blank lines
chomp;
# If the current line ends with a backslash, it is continued on the # following line.
CONTINUATION:
while (/\\\s*$/) { # Check for backslash
my $next_line = <>;
$next_line =~ s/#.*//; # Trim comments
# Otherwise, append it to the current line:
$_ .= $next_line;
chomp;
# If the continuation line ends in a backslash, repeat the loop again }
# Now we have processed comments and continuations; we have assembled # one logical line that might have spanned several physical lines.
my ($key, $value) = split /:\s*/, $_, 2;
if ($key eq 'Name') { # Beginning of info about a new item
$cur_name = $value;
if (exists $property{$cur_name}) {
# This assumes that if you have two sections in your file with # the same name, the second one is just a continuation of the first. # You may want different behavior.
warn "There are two items named `$cur_name'; accumulating data.\n"; }
next LINE;
}
unless (defined $cur_name) {
warn "Unnamed data at the beginning of file at line $.; skipping.\n"; next LINE;
}
# Here's the part that uses references. Gosh, it sure is simple! if (exists $property{$cur_name}{$key}) {
warn "Duplicate `$key' for item `$cur_name' at line $.; skipping.\n"; next LINE;
}
# Those references are practically invisible.
$property{$cur_name}{$key} = $value;
}
Hmm, that's all.
Now we have the data structure built, and it's easy to get what we want:
@names = keys %property; # Get list of all item names
@items = values %property; # Get list of all item hashes
my %properties = %{$property{'Item1'}}; # Get properties of `Item1' print $properties{'Special'}; # `Frank'
So let's do an output something like yours:
# Generate report
foreach $name (sort keys %property) {
print "Item: $name\n";
my $item = $property{$name};
my ($key => $value);
foreach $key (sort keys %$item) {
print "-$key: $$item{$key}\n";
}
}
Hope this helps.
>thanks.
Have a nice day.
|
