REVISED: Variable length array to XML

>>>>> "DP" == Don Pich < XXXX@XXXXX.COM > writes:

  DP> I've been messing with this on and off since my last post.  Here is my 
  DP> current code (cleaned up thanks to advice in this board, and made shorter 
  DP> - Took out some unnecessary arrays):

  DP> use Data::Dumper;
  DP> my $infile = '/media/Docs/Scripts/Perl/Putty/TEST.csv';
  DP> open (CSVFILE, $infile) || die ("Could not open $infile! $!");
  DP> my @final = ();
  DP> my @temp1 = ();
  DP> my @temp2 = ();

the = () are not needed.

  DP> while (my $line = <CSVFILE>) {
  DP>   $line =~ tr/"\r\n//d;
  DP>   $line =~ s/\\/:/g;
  DP>   (my $C1,my $C2,my $C3,my $C4) = split ',', $line;
  DP>   my @temp1 = split (':', $C2);

why do you redeclare @temp1? and temp is ALWAYS a bad name. choose a
name that reflects the actual data or usage of a variable.
  DP>   shift (@temp1);

no comments. you need to say WHY you are doing something like losing an
element from the array.

  DP>   my $tempcount = @temp1;
  DP>   for (my $a = 0 ; $a <= $tempcount-1; $a++) {

$a is reserved for use by sort. also a bad generic name which doesn't
say anything.

  DP>     push ( @temp2, $temp1[$a] );

all you do is copy temp1 to temp2!! that doesn't need a loop.


  DP>   }
  DP>   push ( @temp2, $C1 );
  DP>   push ( @temp2, $C4 );
  DP>   my $temp2count = @temp2;
  DP>   unshift ( @temp2, $temp2count );

with all the temps, this code is impossible to follow. there is no easy
way to tell if it is correct or what.

  DP>   push @final, [ @temp2 ];

since all you do is collect that array and its count:

	push @final, [ scalar @temp2, @temp2 ];

you have a {*filter*} for unneeded temp variables


  DP>   $temp2count = 0;
  DP>   @temp2 = ();
  DP>   @temp1 = ();

all unneeded since the first uses of them assign fresh values to them.


you never use any of the computed data. this doesn't DO anything.

  DP> I'm having a hard time wrapping my head around populating the hash.

a clue. PICK NAMES THAT MEAN SOMETHING. if the xml template has useful
names, why not use those in the code? then if you Data::Dumper the hash
tree, you can compare that to the desired outout and have a chance of
matching them up and fixing it.

also pick a templater to do the main work. look at Template::Simple
which can do this for you with the least amount of work on your part.

uri

-- 
Uri Guttman  ------   XXXX@XXXXX.COM   --------   http://www.**--****.com/  --
-----  Perl Code Review , Architecture, Development, Training, Support ------
---------  Gourmet Hot Cocoa Mix  ----   http://www.**--****.com/  ---------

Yuk!

One declaration with 4 variables is easier to read than 4 declarations.

A regular expression should *look like* a regular expression.

    my($C1, $C2, $C3, $C4) = split /,/, $line;

But sequentially-numbered variable names are almost always a
red flag that indicates you should be using an array instead.

    my @columns = split /,/, $line;




What a worthless choice of variable name!

Choose a more valuable name.

Above you use split without parens, and here you use split with parens.

Decide if you like parens or not, then consistently always use them or
don't use them.

If you don't want the 1st element, then don't save the first element:

    my(undef, @session_parts) = split /:/, $columns[1];




Yet another poorly chosen variable name.

$a is used for sort()ing, so you should not use it for other purposes.

Take the time to come up with _meaningful_ names, it will help
you understand and debug your code more easily.

You should let perl do the indexing for you:

    foreach my $i ( 0..@session_parts ) {




Even better, don't do any indexing at all!

    foreach my $part (@session_parts) {
        push @temp2, $part;
    }


Best is to not even use unnecessary loops:

    @temp2 = @session_parts;


-- 
Tad McClellan
email: perl -le "print scalar reverse qq/moc.liamg\100cm.j.dat/"
The above message is a Usenet post.
I don't recall having given anyone permission to use it on a Web site.

And reduce it further:


while ( my $line = <CSVFILE> ) {
     $line =~ tr/"\r\n//d;
     my @c = split /,/, $line;
     my ( undef, @temp1 ) = split /\\/, $c[ 1 ];
     push @final, [ 2 + @temp1, @temp1, @c[ 0, 3 ] ];
}



John
-- 
Any intelligent fool can make things bigger and
more complex... It takes a touch of genius -
and a lot of courage to move in the opposite
direction.                   -- Albert Einstein

Am 29.03.2011 22:51, schrieb Uri Guttman:

As the one who wrote that piece of code I've got to add my
2 Ct here. Perl does do autovivication, but in this case it
will not DWIM.
     $ptr = $ptr->{$node};
will *not* autovivicate a hash entry with the key "$node". There is
neither an assignment to the hash entry, nor does the code try to
access a deeper entry that would bring intermediate entries to life.

Making $ptr point to an undef value (that would happen with each
iteration if the hash ref wasn't assigned manually), perl *would*
autovivicate a hash reference - but it would have no association
with our %hash, and only in the last inner iterations would the
(short-lived) referenced hash even hold an entry with the key "$node".

Or more concise from perlfaq4:
| Normally, merely accessing a key's value for a nonexistent key
| does *not* cause that key to be forever there.

-Chris

>>>>> "CW" == Christian Winter < XXXX@XXXXX.COM > writes:

  CW> Am 29.03.2011 22:51, schrieb Uri Guttman:
  >>>>>>> "DP" == Don Pich< XXXX@XXXXX.COM >  writes:
  >> 
  DP> my %hash;
  DP> foreach my $leaf (@final) {
  DP> my $ptr = \%hash;
  DP> foreach my $i ( 0 .. $#$leaf - 1 ) {
  DP> my $node = $leaf->[$i];
  DP> if( $i != $#$leaf - 1 ) {
  DP> $ptr->{$node} = {}
  DP> unless( exists $ptr->{$node} );
  DP> $ptr = $ptr->{$node};
  DP> } else {
  DP> $ptr->{$node} = $leaf->[$i + 1];
  >> 
  >> learn about autovivification. there is no need to manually make a hash
  >> ref if one doesn't exist. assume it does and perl creates it for you.

  CW> As the one who wrote that piece of code I've got to add my
  CW> 2 Ct here. Perl does do autovivication, but in this case it
  CW> will not DWIM.
  CW>     $ptr = $ptr->{$node};
  CW> will *not* autovivicate a hash entry with the key "$node". There is
  CW> neither an assignment to the hash entry, nor does the code try to
  CW> access a deeper entry that would bring intermediate entries to life.

true. i didn't delve into the logic much. it was the 
	$ptr->{$node} = {}
line which i was triggered on. that is almost never needed due to
autoviv. the fact that his code didn't use that was confusing. in fact
it is odd what he is doing there. why do $ptr = $ptr->{$node}; ? it just
gets the new anon hash into $ptr. i can't make head nor tail of what he
is trying to do in that loop.

  CW> Making $ptr point to an undef value (that would happen with each
  CW> iteration if the hash ref wasn't assigned manually), perl *would*
  CW> autovivicate a hash reference - but it would have no association
  CW> with our %hash, and only in the last inner iterations would the
  CW> (short-lived) referenced hash even hold an entry with the key "$node".

i was making that same point in a recent thread on autoviv and i know it
well. the code is just too odd looking anyhow. as i keep telling him,
use a templater.

uri

-- 
Uri Guttman  ------   XXXX@XXXXX.COM   --------   http://www.**--****.com/  --
-----  Perl Code Review , Architecture, Development, Training, Support ------
---------  Gourmet Hot Cocoa Mix  ----   http://www.**--****.com/  ---------

Am 30.03.2011 07:49, schrieb Uri Guttman:
 >

It's basically just like a pointer in a C-style tree. At first
it points to the root node, then with each iteration a child node
gets added (if not already there) and the pointer is moved to point to
this child. The fact that it uses the name of a hash key instead of
a C struct member's name is just a variation of the concept.


I'm not convinced that a template system is the only answer to
the problem. A CSV module and Hash::Merge would be enough to populate
the hash structure, and one of the numerous xml modules should be
able to output what he wants - though that could be done in a few
lines of code as well. In fact, it should be nothing more than

dump_hash(\%hash);

sub dump_hash
{
   my($h, $ind) = @_;
   my $indent = "  " x $ind;
   return (ref $h eq "HASH") ?
     join('', map {
       $indent . "<$_>" . $/ .
       dump_hash($h->{$_}, $ind + 1) .
       $indent . "</$_>" . $/
     } keys %$h)
     :
     $indent . $h . $/;
}

I myself would, of course, use appropriate modules for production
code which check for corner cases and provide sensible error messages,
but the OPs problem and the possible solutions do make for a fine
academic task to learn about (or toy around with) topics like hashes,
references, recursion, top-down vs. bottom-up concepts or variable
depth algorithms.

-Chris

>>>>> "CW" == Christian Winter < XXXX@XXXXX.COM > writes:

  CW> Am 30.03.2011 07:49, schrieb Uri Guttman:
  >>>>>>> "CW" == Christian Winter< XXXX@XXXXX.COM >  writes:
  >> 
  >> true. i didn't delve into the logic much. it was the
  >> $ptr->{$node} = {}
  >> line which i was triggered on. that is almost never needed due to
  >> autoviv. the fact that his code didn't use that was confusing. in fact
  >> it is odd what he is doing there. why do $ptr = $ptr->{$node}; ? it just
  >> gets the new anon hash into $ptr. i can't make head nor tail of what he
  >> is trying to do in that loop.

  CW> It's basically just like a pointer in a C-style tree. At first
  CW> it points to the root node, then with each iteration a child node
  CW> gets added (if not already there) and the pointer is moved to point to
  CW> this child. The fact that it uses the name of a hash key instead of
  CW> a C struct member's name is just a variation of the concept.

i know that concept well. why he needed to assign {} is the
question. you can use autoviv to build that sort of thing.

  >> i was making that same point in a recent thread on autoviv and i know it
  >> well. the code is just too odd looking anyhow. as i keep telling him,
  >> use a templater.

  CW> I'm not convinced that a template system is the only answer to
  CW> the problem. A CSV module and Hash::Merge would be enough to populate
  CW> the hash structure, and one of the numerous xml modules should be
  CW> able to output what he wants - though that could be done in a few
  CW> lines of code as well. In fact, it should be nothing more than

  CW> dump_hash(\%hash);

  CW> sub dump_hash
  CW> {
  CW>   my($h, $ind) = @_;
  CW>   my $indent = "  " x $ind;
  CW>   return (ref $h eq "HASH") ?
  CW>     join('', map {
  CW>       $indent . "<$_>" . $/ .
  CW>       dump_hash($h->{$_}, $ind + 1) .
  CW>       $indent . "</$_>" . $/
  CW>     } keys %$h)
  CW>     :
  CW>     $indent . $h . $/;
  CW> }

  CW> I myself would, of course, use appropriate modules for production
  CW> code which check for corner cases and provide sensible error messages,
  CW> but the OPs problem and the possible solutions do make for a fine
  CW> academic task to learn about (or toy around with) topics like hashes,
  CW> references, recursion, top-down vs. bottom-up concepts or variable
  CW> depth algorithms.

a templater would be easier to see the xml layout (and xslt is just a
super fancy templater of sorts). but the code he used to descend the
tree was horrible in all sorts of ways.

and your code doesn't handle arrays which his data has. Template::Simple
can do all that with almost no effort.

uri

-- 
Uri Guttman  ------   XXXX@XXXXX.COM   --------   http://www.**--****.com/  --
-----  Perl Code Review , Architecture, Development, Training, Support ------
---------  Gourmet Hot Cocoa Mix  ----   http://www.**--****.com/  ---------