tags:

views:

114

answers:

3

This is the first time I have manipulated hashes and arrays in this way -- and it is working. Basically, for every key there are multiple values that I want to record and then print out in the form "key --> value --> value --> val..."

My code is as follows. I am surprised that it works, so concerned that it works "by mistake". Is this the correct way to accomplish this task, or is there a more efficient or appropriate method?

while ($source =~ m/(regex)/g) { #Get all key names from source
    $listkey = $1; #Set current list key to the current regex result.
    $list{$listkey} = ++$i unless $list{$listkey}; #Add the key to the hash unless it already exists.
    $list{$listkey} = [] unless exists $list{$listkey}; #Add an array for the hash unless the hash already exists.
    while ($loopcount==0) {
            if ($ifcount==0) {
                    $listvalue=result_of_some_function_using_list_key; #Get the first list value by using the list key.
                    $ifcount++; #Increment so we only get the first list value once.
            } else {
                    $listvalue=result_of_some_function_using_list_value; #Update the list value by using the last list value.
            }
            if ($listvalue) { #If the function returned a value...
                    push @{$list{$listkey}}, $listvalue; #...then add the value to the hash array for the key.
            } else { #There are no more values and we need a new key.
                    $listkey=0; #Reset variable.
                    $listvalue=0; #Reset variable.
                    $loopcount++; #Increment loop counter to exit loop.
            }
    }
$ifcount=0; #Reset count variable so the next listvalue can be generated from the new key.
    $loopcount=0; #Reset count variable so another loop can begin for a new key.
}
foreach $listkey (keys %list) { #For each key in the hash.
    print "$listkey --> "; #Print the key.
    @values = @{$list{$listkey}}; #Reference the arrays of the hash.
    print join ' --> ', @values; #Print the values.
    print "\n"; #Print new line.
}
+1  A: 

Nope! If this works, it's definitely "by mistake". But it's also obvious that this isn't your real code and that you added several more mistakes in "translating" it to an example, so it's hard to judge exactly what the intent was, but going from the skeleton of your program, it looks like it should be something like:

my %result;

while ($source =~ m/(regex)/g) {
  my $key = $1;
  my $value = mangle($key);
  while ($value) {
    push @{ $results{$key} }, $value;
    $value = frob($value);
  }
}

and no more. Your attempts to initialize the hash aren't doing what you think they are (and aren't necessary), your while loop as written isn't a good idea at all, and neither are all the global variables.

hobbs
+2  A: 

The code above has many unnecessary steps. Perl is a very expressive language, and allows logic like this to be expressed very simply:

# uncomment for some sample data
# sub function {"@_" !~ /^\[{3}/ and "[@_]"}
# my $source = 'one two three';

my %list;
while ($source =~ m/(\S+)/g) {
    my $key   = $1;
    my $value = function($key);

    while ($value) {
        push @{ $list{$key} }, $value;
        $value = function($value)
    }
}

for my $key (keys %list) {
    print join(' --> ' => $key, @{$list{$key}}), "\n";
}
Eric Strom
agreed. generally, in Perl you should only ever see loop indexes (the `for ($i = 0...` C-style loop) or counters in cases where you actually need to do something with those values. counters and indexes are also often a good source of awkward-to-spot bugs
plusplus
+2  A: 

The following code does the same as your code, without the unnecessary steps.

while ($source =~ m/(regex)/g) { # Get all key names from source
    $listkey = $1;            # Grab current regex result.
    $listvalue = result_of_some_function_using_list_key;
    while ($listvalue) {
        push @{$list{$listkey}}, $listvalue; 
        $listvalue = result_of_some_function_using_list_value;
    }
    $listkey = 0;                # Reset variable.
    $domain = 0;                 # Reset variable.
}   

However, as others have commented, global variables should be avoided in most cases. Instead, the list key and list value should be lexically scoped with my(), and the functions for generating list values should take one or more parameters (domain, list key and/or list value) as input.

The lines

$list{$listkey} = ++$i unless $list{$listkey};
$list{$listkey} = [] unless exists $list{$listkey};

in your original code aren't needed, it is sufficient with push @{ $list{$key} }, $value to initialize an entry.

markusk
Thanks, I have a better understanding of where I went wrong after the brief explanation.
Structure