Help to improving my code please?

spacey has asked for the wisdom of the Perl Monks concerning the following question:

Hello to all,
Would somebody be kind enough to pick and poke at this code?

Let me try and explain what I’m trying to achieve.
I have a file containing a list of filenames and other information such as graphic locations.

Having read the file information and done some processing on it.
I want to sort it into alphabetical order for later outputting by letter.
For example all “A” file will be printed into part of a html table under “A”
“B” will go into another part of a html table etc.

I have manage to do this but (I know always a but) I’m sure that the way im doing it is no way the best solution.
As I end up having 26 arrays and 26 if else statements and lastly 26 output statement.
I have thought that maybe I could “join” each alphabetical list together as one string and then place it into one array for later outputting.
Before I embark on this I would welcome anybody suggestions and comments.

Regards,
Gareth

sub read_db_list {

open (FILELIST, "<$filelistdb") or die "Sorry File list not available"
+;

        foreach $fileinlist (<FILELIST>) {
        chomp($fileinlist);
        ($path,$file_type,$file_name,$file_image,$file_description) = 
+split(/:/,$fileinlist);

         $infoline = "<a href=\"$path/$file_name\"><img src=\"images/$
+file_image\" width=\"128\" height=\"128\" alt=\"$file_description\"><
+/a><p>";

            
#### I now find what letter filename start with in order to assign it 
+to A-Z list #####
    if ($file_name =~m/^A/i) {
                push(@arrayA,$infoline);
                } elsif ($file_name =~/^B/i) {
                push(@arrayB,$infoline);
                }
###### What I would like to know is there a better way other then doin
+g the above if statements 26 time for each letter. Ending up with 26 
+arrays?
    

        }
}
[download]

Comment on Help to improving my code please? Download Code

Replies are listed 'Best First'.
Re: Help to improving my code please? by Zaxo (Archbishop) on May 30, 2003 at 12:14 UTC
The problem of repeating all those pushes is not necessary. A nice data structure like a hash will settle that: `sub read_db_list { my %listdb; # ... in the read loop # replacing the if block push @{$listdb{ uc substr( $file_name, 0, 1) }}, $infoline; # after all is read return \%listdb; }` [download] The %listdb hash automatically has as many keys as your files have initial characters. Those keys are created as needed by uppercasing the first character of the name, which is extracted by substr. By treating the hash value as an array reference, it automagically becomes one, giving you a hash of arrays - affectionately known as HoA. To print all that you may want to look at one of the template modules, or else use CGI.pm's handy html generation functions. Those are very nifty with array references. After Compline, Zaxo	[reply] [d/l]
Re: Help to improving my code please? by Skeeve (Parson) on May 30, 2003 at 12:05 UTC
how about this: `($firstLetter)= ($file_name=~ /^(.)/); push(@{$hash{uc($firstletter)}}, $infoline);` [download] This will give you a hash of arrays: `$hash{'A'} -> [ infolineA1, infolineA2 ... ] $hash{'B'} -> [ infolineB1, infolineB2 ... ]` [download] with this loop `foreach $letter (sort keys %hash) { print $letter; foreach $infoline (@{$hash{$letter}}) { print $infoline; } }` [download] you can output your lists. Note that I've added a uc() to $firstletter above, giving you just upper case. This might not be the best way to do it, but its at least one way.	[reply] [d/l] [select]
Re: Help to improving my code please? by zby (Vicar) on May 30, 2003 at 12:14 UTC
You can use a hash of arrays structure: `$file_name =~ /^[A-Z]/i; push @{$arrays->{$1}}, $infoline;` [download] This way all the dispatching is done by the hash not by ifels switching. Then you need to know how to use it: `for $key (keys %$arrays){ for $element (@{$arrays->{$key}}){ print $element; } }` [download] Read more about this subject in `perldoc perlre`.	[reply] [d/l] [select]
Re: Help to improving my code please? by moxliukas (Curate) on May 30, 2003 at 12:02 UTC
A first thought that comes into my mind would be to construct a hash of arrays (HoA) to get rid of those 26 arrays that you have (and what if the filename doesn't start with a letter at all?) here is some code: `$hash{$_} = [] for ('A'..'Z'); # prepare some empty arrayrefs $file_name =~ /^(.)/; # Mmm... It would be better to use a substr here + :/ push @{$hash{uc($1)}}, $infoline; # push an $infoline, first by upper +casing the key` [download] I am no Perl guru, so others will probably point to a better direction.	[reply] [d/l]
Re: Help to improving my code please? by cciulla (Friar) on May 30, 2003 at 12:46 UTC
Doin' thumbnails, eh? How about a control break? Read more... (2 kB)	[reply] [d/l]
Re: Help to improving my code please? by spacey (Scribe) on May 30, 2003 at 13:39 UTC
Thanks to one and all, Your answers make much sense. I will have a poke around on my code. Again Thank you all regards Gareth	[reply]