[Pdx-pm] Different data structures for Hash of array, same code (mostly)

[Shlomi Fish]

Hi Daniel!

On Wednesday 18 Nov 2009 17:12:07 Daniel Herrington wrote:
> All,
> 
> Sorry if this is a double post, but I didn't get a confirmaiton that my
> last email made it.

It did.

> 
> I have two code snippets that should do the same thing, but do not
> produce the same data structure. I can't seem to figure out why.
> 

It seems your code snippets contain many anti-patterns. Let me say what I find 
wrong with them. If you wonder why a certain portion of code does not work as 
it should, you can always use the perl debugger interactively:

http://perl-begin.org/topics/debugging/

> Code A:
> ...
>                if ($boxNameH{$boxName}) {

First I should note that I find these code snippets hard to read. They do not 
contain any empty lines, separating code paragraphs, and the variable names 
are given in camelCase or CamelCase with weird System hungarian notation (not 
even Apps Hungarian - see:
http://en.wikipedia.org/wiki/Hungarian_notation#Systems_vs._Apps_Hungarian
), with a significant leading intendation which makes it hard to follow inside 
an email and "#Comments" instead of "# comments".

Now let me continue.

>                    #If we already have a list created
>                    my $refBoxName = $boxNameH{$boxName};
>                    my @derefArray = @$refBoxName;
>                    $derefArray[0]++;
>                    push (@derefArray, $jobName);
>                    $boxNameH{$boxName} = \@derefArray;

OK, why are you dereferencing an array and just replacing it. You can just do 
(untested):

<<<<
my $box = $boxNameH{$boxName};
$box->[0]++;
push @$box, $jobName;
>>>

Much easier, shorter and clearer. 

I should note that doing $box->[0]++ seems redundant in this case. Are you 
counting the number of jobs there? You can easily determine it based on 
scalar(@$box).

>                    $caJIL{$jobName}{"$jilCommand"} = "$jilValue";

Why are you using "$jilCommand" And "$jilValue" in quotes. Normally the quotes 
are not needed (they are when you are trying to stringify objects. What is the 
meaning of the jil prefix?

>                } else {
>                    #We're the first one
>                    my @derefArray;
>                    $derefArray[0] = 1;
>                    push (@derefArray, $jobName);
>                    $boxNameH{$boxName} = \@derefArray;
>                    $caJIL{$jobName}{"$jilCommand"} = "$jilValue";
>                }

Again, you don't need to count the number of jobs explicitly.

> ...
> Using Dumper, the data in %boxNameH is:
> 3108$VAR1 = {
>          'CPO_TNC_WCONV0' => [
>                                2,
>                                'CPO_TNC_WCONVERSION_CHMOD',
>                                'CPO_TNC_WCONVERSION_SETUP'
>                              ],
>          'CURRENTLINK_25' => [
>                                1,
>                                'CURRENT_LINK_25'
>                              ],
> ...
> 
> Code B:
> ...
>    my %newBoxNameH;
>    foreach (@arBoxNames) {
>        my $boxName = $_;

Why are you doing such a weird loop? Just do:

<<<<
foreach my $boxName (@arBoxNames) {
 ...
}
>>>>

No need to loop over @arBoxNames with the default variable.

>        my $newBoxName;
>        my @jobNamesList;
>        if ($ref_hlqMap->{$boxName}) {
>            $newBoxName = $ref_hlqMap->{$boxName} ;        } else {
>            $newBoxName = $boxName;
>        }

You need to fix your indentation here. Prefarably use:

<<<
$newBoxName = $ref_hlqMap->{$boxName} || $boxName;
>>>

>        print __LINE__ . " $newBoxName\n";

Why not use STDERR, or a logging module?

>        my $ref_arJobNamesB = $ref_boxNameH->{$boxName};

Implement my commentaries to your code below too.

Both of these code snippets seem entirely different to me at first glance. 
Your first one has no loops.

Regards,

	Shlomi Fish


>        my @arJobNamesB = $ref_arJobNamesB;
>        foreach (@arJobNamesB) {
>            my $jobName = $_;
>            my $newJobName;
>            if ($ref_hlqMap->{$jobName}) {
>                $newJobName = $ref_hlqMap->{$jobName};
>            } else {
>                $newJobName = $jobName;
>            }
>            if ($newBoxNameH{$newBoxName}) {
>                my $ref_arJobNames = $newBoxNameH{$newBoxName};
>                my @arJobNames = @$ref_arJobNames;
>                push (@arJobNames,$newJobName);
>                $newBoxNameH{$newBoxName} = \@arJobNames;
>            } else {
>                my @arJobNames;
>                push (@arJobNames,$newJobName);
>                $newBoxNameH{$newBoxName} = \@arJobNames;
>            }
>        }
> ...
> 289$VAR1 = {
>          'CURRENTLINK_25' => [
>                                [
>                                  1,
>                                  'CURRENT_LINK_25'
>                                ]
>                              ],
>          'CPO_TNC_WCONV0' => [
>                                [
>                                  2,
>                                  'CPO_TNC_WCONVERSION_CHMOD',
>                                  'CPO_TNC_WCONVERSION_SETUP'
>                                ]
>                              ]
>        };
> ...
> 
> You can see in Code B's dump that the list is actually embedded another
> layer down as if Code B produces a Hash of Array of Array. I just can't
> seem to figure out why this is happening?
> 
> thanks,
> 

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Optimising Code for Speed - http://shlom.in/optimise

Chuck Norris read the entire English Wikipedia in 24 hours. Twice.