in reply to Thanks !! (XML + Excel) with (win32::OLE and XML::Simple)

Here're my code review comments:
  1. The opendir/readdir/closedir lines become one using glob or something like File::Find::Rule.
  2. $dir is set, but then "." is hardcoded in the copendir.
  3. Prompt text says "all the xml files in the scripts folder" but the script looks for ./*.xml 
  4. my $a = "x";            
while ($a ne "y")
{
    ...
    chop $a;
}

    [download]
    can be: while( my $input ne "y" ){ ... ; chomp $input; }
    Avoid $a and $b as var names, since they are non-descriptive, and have special meaning (see sort).
    Don't blindly chop -- use chomp.
  5. Or, just remove that whole while block ... if they didn't wany it run, they shouldn't have executed the script :)
    (yes, you could argue that they ran the script accidentally. Well, then i'd say it's just as likely they accidently hit "y" when they meant "n" to the confirmation, and you should ask again. and again. and .... :) ) Just makes the script have less moving parts (reaD: easier to maintain). 
  6. unless ($files[0]){
    print "\nSorry, no xml file(s) in script folder\n\nScript Terminat
+ed...\n\n";
    }
    else{
    ...  # a gazillion rows
}

    [download]
    This should read (use array in scalar context; and avoid large block): 
    unless( scalar @files ){
  warn "no files";
  exit;
}
...

    [download]
    You may even want to die instead.
  7. You might want to make a parse_file() sub, so the main code then becomes: 
    parse_file($_) for @files;

    [download]
  8. for my $i(0 .. $count) {
    This is most commonly written as 
    foreach my $i ( 0 .. $count ){

<li> to be more explicit (since you want an array), do <c>unless( ref(
+$doc->{$sub1}) eq 'ARRAY' ){

    [download]

    Does it even happen that these aren't arrays? if so, nod. if not, maybe just ditch that clause (and if it ever happens by mistake, it'll just error out).
  9. I think the nested loops become easier to deal with if you work with array values instead of indexes ... something like: 
    foreach my $sub1 ( sort keys %{ $doc } ){

  ... # $sub1

    foreach my $sub2Hash ( @{ $doc->{$sub1} } ){

        for my $sub2 ( sort keys %$sub2Hash ){

            unless (ref($sub2Hash{$sub2})) {

                ...  # $sub2
                $worksheet->Cells($rowCount, $colCount)->{Value} = $su
+b2;
                $worksheet->Cells($rowCount, $colCount)->{Value} = $su
+b2Hash{$sub2};
                next;

            }

            foreach my $sub3Hash( @{$sub2Hash{$sub2}} ){

                for my $sub3 ( sort keys %$sub3Hash ){

    [download]
  10. BUT, I notice that, at each nested level, it all looks the same -- should break that out into a function. Here's something that does that, and the previous comment about using variables as you go: 
    sub __add_non_array {
  my ($worksheet, $rowCount, $colCount, $sub) = @_;
  $worksheet->Cells($rowCount, $colCount)->{Value} = $sub;
  $worksheet->Cells($rowCount, $colCount)->Borders() -> {Weight} = 2;
  $colCount++;
  $worksheet->Cells($rowCount, $colCount)->{Value} = $doc->{$sub};
  $worksheet->Cells($rowCount, $colCount)->Borders() -> {Weight} = 2;
  $rowCount++;
  $colCount--;
  return ($rowCount, $colCount);
}

while( my ($sub1, $aoh1) = each %$doc ){

    unless (ref($aoh1) eq 'ARRAY') {
      ($rowCount,$colCount) = __add_non_array($worksheet, $rowCount, $
+colCount, $sub1);
      next;
    }

    foreach my $sub2Hash ( @$aoh1 ){

       while( my ($sub2, $aoh2) = each %$sub2Hash ){

          unless (ref($aoh2) eq 'ARRAY') {
            ($rowCount,$colCount) = __add_non_array($worksheet, $rowCo
+unt, $colCount, $sub2);
            next;
          }

            foreach my $sub3Hash ( @$aoh2 ){

                while( my ($sub3, $aoh3) = each %$sub3Hash ){

                  unless (ref($aoh3) eq 'ARRAY') {
                    ($rowCount,$colCount) = __add_non_array($worksheet
+, $rowCount, $colCount, $sub3);
                    next;
                  }

                    foreach my $sub4Hash ( @$aoh3 ){
                        print "Collecting general information...\n";

                        while( my ($sub4, $aoh4) = each %$sub4Hash ){

                          unless (ref($aoh4) eq 'ARRAY') {
                            ($rowCount,$colCount) = __add_non_array($w
+orksheet, $rowCount, $colCount, $sub4);
                            next;
                          }

                            print "Collecting $sub4 ...\n";

                            $subDataRow = scalar(@$aoh4) + ($rowCount 
++ 1);

                            my $worksheet = $workbook->Worksheets->Add
+({After=>$workbook->Worksheets($workbook->Worksheets->{Count})});
                            $worksheet -> {Name} = $sub4;
                            $worksheet -> Range("A1") -> Font -> {Size
+}= 14;
                            $worksheet -> Range("A1") -> Font -> {Colo
+rIndex}= 2;
                            $worksheet -> Range("A1") -> {Value} = "$s
+ub4";
                            $worksheet -> Range("A1:E1") -> Merge;
                            $worksheet -> Range("A1:E1") -> Interior -
+> {ColorIndex} = 25;
                            $worksheet -> Range("A1:E1") -> Borders() 
+-> {Weight} = 3;
                            my $subDataCol = 1;

                            foreach my $sub5Hash ( @$aoh4 ){

                                my $temp = "";

                                while( my ($sub5, $aoh5) = each %$sub5
+Hash ){
                                    my $curID = $sub5Hash{enclosureNum
+ber} || $sub5Hash{number} || 0;;
                                    my $nextup = "";

                                                     if(ref($aoh5) eq 
+'ARRAY'){
                                                     #################
                                                     # SUBINFORMATIE #
                                                     #################
+  $sub 4 is rank $sub5 is attribuut van rank $sub6 is element binnen 
+rank eg arrayref

                                                         foreach my $s
+ub6Hash ( @$aoh5 ){

                                                                 # Nie
+uwe ArayRef, ExtentbyVol,StorageDeviceFRY (nieuwe titels maken

                                                                 if($n
+extup ne $sub5){
                                                                     $
+new = 1;

                                                                     $
+subDataRow++;
                                                                     $
+subHeaderRow = $subDataRow;
                                                                     $
+subDataCol=1 ;
                                                                     $
+nextup = $sub5;
                                                                 }
                                                                 else 
+{$new = 0;}

                                                $subDataRow++;

                                                              while( m
+y ($sub6, $val6) = each %$sub6Hash ){

                                                        # subinfo titl
+e on new element
                                                        if ($temp ne $
+curID){

                                                            $temp = $c
+urID;

                                                            $worksheet
+->Cells($subDataRow, $subDataCol)->{Value} = "$sub4 : $curID";
                                                            $worksheet
+->Cells($subDataRow, $subDataCol)->Interior->{ColorIndex} = 33;
                                                            $worksheet
+->Cells($subDataRow, $subDataCol)->{Font}->{Bold} = 1;
                                                            $worksheet
+ ->Cells($subDataRow, $subDataCol)-> Borders() -> {Weight} = 1;
                                                            $subDataRo
+w+=2;
                                                            $subHeader
+Row = $subDataRow;
                                                            $subDataRo
+w++;
                                                        }

                                                        # Header
                                                         if($new){
                                                            $worksheet
+->Cells($subHeaderRow, $subDataCol)->{Value} = $sub6;
                                                            $worksheet
+->Cells($subHeaderRow, $subDataCol)->{Font}->{Bold} = 1;
                                                            $worksheet
+ ->Cells($subHeaderRow, $subDataCol)-> Borders() -> {Weight} = 2;
                                                            $worksheet
+ ->Cells($subHeaderRow, $subDataCol)-> Borders() -> {ColorIndex} = 25
+;
                                                         }

                                                            $worksheet
+->Cells($subDataRow, $subDataCol)->{Value} = $val6;
                                                        $subDataCol++;
                                                                     }
                                            $subDataCol=1;
                                                             }
                                                     }

                                                     else{  # aoh5 is 
+not an array

                                                     #################
+##
                                                     # MAIN INFORMATIO
+N#
                                                     #################
+##


                                                         # Index, if a
+ny of those, will be placed in first column
                                                         if ($sub5 eq 
+"number" || $sub5 eq "id" || $sub5 eq "enclosureNumber"){
                                                             $workshee
+t->Cells($mainHeaders, 1)->{Value} = $sub5;
                                            $worksheet->Cells($mainHea
+ders, 1)->{Font} -> {ColorIndex} = 25;
                                            $worksheet->Cells($mainHea
+ders, 1)->{Interior} -> {ColorIndex} = 15;
                                            $worksheet->Cells($mainHea
+ders, 1)->Borders() -> {Weight} = 2;
                                            $worksheet->Cells($mainHea
+ders, 1)->{Font}->{Bold} = 1;
                                                      $worksheet->Cell
+s($rowCount, 1)->{Value} = $aoh5;
                                            $worksheet->Cells($rowCoun
+t, 1)->{Font}->{Bold} = 1;
                                            $worksheet->Cells($rowCoun
+t, 1)->Borders() -> {Weight} = 2;
                                                         }

                                                         else{
                                                             unless ($
+worksheet->Cells($mainHeaders, $colCount)->{Value}){

                                                                 $work
+sheet->Cells($mainHeaders, $colCount)->{Value} = $sub5;
                                                        }

                                                    if ($sub5 eq "wwpn
+"){
                                                $worksheet->Cells($row
+Count, $colCount)->{NumberFormat} = "0";
                                                $worksheet->Cells($row
+Count, $colCount)->Borders() -> {Weight} = 2;

                                                    }

                                                             $workshee
+t->Cells($rowCount, $colCount)->Borders() -> {Weight} = 2;
                                                             $workshee
+t->Cells($rowCount, $colCount)->{Value} = $aoh5;
                                                             $workshee
+t->Cells($mainHeaders, $colCount)->Borders() -> {Weight} = 2;
                                            $worksheet->Cells($mainHea
+ders, $colCount)->{Font}->{Bold} = 1;
                                            $worksheet->Cells($mainHea
+ders, $colCount)->{Font} -> {ColorIndex} = 25;
                                            $worksheet->Cells($mainHea
+ders, $colCount)->{Interior} -> {ColorIndex} = 15;
                                            $colCount++;
                                            $LastRow = $worksheet->Use
+dRange->Find({What=>"*", SearchDirection=>xlPrevious, SearchOrder=>xl
+ByRows})->{Row};
                                        }
                                    }
                                } $rowCount++;
                                  $subDataRow++;
                                  $colCount=2;
                                  $worksheet -> Range("A:X") -> {Colum
+ns} -> Autofit;

                            }
                              $subDataRow=$rowCount+2;;
                              $colCount=2;
                              $rowCount=4;

                        #Seperate general information on first sheet.
                        } $rowCount=4; $colCount+=3;
                    }
                } $rowCount=4; $colCount+=3;
            }
        } $rowCount=4; $colCount+=3;
    }
}

    [download]
    NOTE: I noticed after i wrote it out that the while loses the sort keys functionality you had. If that's important, you can replace while( my ($sub2, $aoh2) = each %$sub2Hash ){ with: 
      foreach my $sub2 ( sort keys %$sub2Hash ){
    my $aoh2 = $sub2Hash->{$sub2};

    [download]

    Note: i also feel like there should be a much more elegant (& shorter/less cumbersome) solution maybe recursing through those 6 levels .. at least through the first ~4 or 5. (don't have time at the moment to write it out)

Replies are listed 'Best First'.
Re^2: Thanks !! (XML + Excel) with (win32::OLE and XML::Simple)
by Sporti69 (Acolyte) on Dec 06, 2008 at 18:23 UTC
    Nice feedback !!!

    I applied your first 8 remarks to the script.

    Your remark 9: well level 1,2 and 3 are put into one sheet and thus they are arrays, but all the other differ. Even in the deepest level it can be eighter array or hash. I hope you (guys) ran the script to give it a go.

    I cannot try out the other suggestions yet because of the error written below, strange

    I will paste the updated version, BUT I have an error now saying: Couldn't open encmap iso-859-1.enc: No such file or directory at C:/Perl/lib/XML/Parser.pm line 187 I dont get this ? It finds the file as you can see in the output, and I do not use the parser module in the script.

    What's wrong, or do only I have this error all of a sudden.

    XML File is in the first post, this is the updated script:
    Read more... (25 kB)
[reply]
[d/l]
Re^2: Thanks !! (XML + Excel) with (win32::OLE and XML::Simple)
by Sporti69 (Acolyte) on Dec 07, 2008 at 20:09 UTC
    Hey all,

    I added the 10 comments you made. Thanks again!

    You oversaw a few things in the suggestions. I added an variable to the function because the sublevel wasn't enough, it was the attributes title, not the value

    Only problem is that your suggestion doesn't keep them sorted any more

    Anyways I changed th sub into level, and some other variables, I renamed more obvious. Documented the code aswell.

    Maybe you can see some other shortcuts now the code looks very nice.

    Read more... (25 kB)

    And the XML again

    Read more... (30 kB)

    To all

    How can we make this even better ?
[reply]
[d/l]
[select]
      Only problem is that your suggestion doesn't keep them sorted any more
      I noted that i at the bottom of my suggestion (after the code blocks), w/how to fix it:
      NOTE: I noticed after i wrote it out that the while loses the sort keys functionality you had. If that's important, you can replace while( my ($sub2, $aoh2) = each %$sub2Hash ){ with: 
        foreach my $sub2 ( sort keys %$sub2Hash ){
    my $aoh2 = $sub2Hash->{$sub2};

      [download]
[reply]
[d/l]
[select]
        I did change the code like you said. Maybe not completely... because $sub2Hash doesn't exist, its $doc->{$level1}$i in stead of only $sub2Hash. Or did you see it otherwise ?
[reply]

In Section Seekers of Perl Wisdom