Front page | perl.beginners |
Postings from July 2011
Hi Leo,
I'm commenting on your code below because it exhibits some bad elements.
On Wed, 13 Jul 2011 11:00:33 -0700
Leo Susanto <leosusanto@gmail.com> wrote:
> Mohan,
>
> I would suggest you learning perl hash: http://perl101.org/hashes.html
>
Also see:
http://perl-begin.org/topics/hashes/
> #!/usr/bin/perl
> use Text::CSV;
> use DBI;
> use Data::Dumper;
There is no "use strict;" and "use warnings;" at the beginning of the file:
http://perl-begin.org/tutorials/bad-elements/
>
> # CONFIG VARIABLES
> ...
> # DATA SOURCE NAME
> ...
Why is this in all-capital-letters?
>
> my @regions = ('east','north','south','west');
> my @statuses_string = ('pws','open','hold','pwu','reopen');
Why did you put them in lowercase letters? This will allow people to enter
"sOUth" or other stuff like that. Better check for the existence of the exact
string, and if so throw an exception.
BTW, "@statuses_string" should be "@status_strings" probably.
> my %region_data;
> foreach my $region (@regions) {
> foreach my $status_string (@statuses_string) {
> $region_data{$region}{status}{$status_string}{count} = 0;
> }
> $region_data{$region}{total_count} = 0;
> }
You can write this as:
my %region_data = (map { $_ => { total_count => 0,
status => { map { $_ => { count => 0 } } @status_strings },
} } @regions);
> print Dumper \%region_data; # inspect the pristine data structure
>
> sub ByRegion
> {
> my @columns=@_;
If you're passing the columns you should pass them as an array reference.
> $regions{ lc($columns[1]) }{status}{ lc($columns[2]) }c++;
> $regions{ lc($columns[1]) }{total_count}++;
1. The "c++" here is a typo.
2. You should unpack the columns:
my (undef, $region, $status) = @$columns;
3. You reference $regions{lc($columns[1])} twice.
Regards,
Shlomi Fish
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Why I Love Perl - http://shlom.in/joy-of-perl
Stray XSLT code causes more deaths than road accidents.
Please reply to list if it's a mailing list post - http://shlom.in/reply .
Thread Previous
|
Thread Next