On Fri, Jun 12, 2009 at 20:24, Steve Bertrand<st...@ibctech.ca> wrote: > Hi all, > > I'm still in the process of updating/replacing some accounting > applications, and thought I'd ask for a peer review. snip
My version of your code is available at http://codepad.org/jrP45bUp It has not been tested because I don't have the right environment for it, but it does pass perl -c. What follows are the notes I took while reviewing your code. > my $config = Config::Tiny->new(); > $config = Config::Tiny->read( '/usr/local/etc/eagleaccounting.conf'); You don't need to call the new method if you are calling the read method (it returns a new object), and you need to check to see if it was able to create an object (in case the file doesn't exist, can't be read, has the wrong format, etc). my $config = Config::Tiny->read('/usr/local/etc/eagleaccounting.conf') or die "an error occurred while reading the config file: $Config::Tiny::errstr"; > my $lib_dir = $config->{'global'}->{'lib_dir'}; > push (@INC, $lib_dir); This is happening way to late to be useful, and you should almost never modify @INC directly (use the lib pragma instead). I also don't see why you are doing it. All of the modules have been loaded when this code runs, and you have no calls to require. > my $confirm_debug_run; This variable is in the wrong scope. Move it into the if. In fact, you should probably just say my $confirm_debug_run = <STDIN>; > $recipient = 'st...@ibctech.ca'; It is a good practice to create a dev email account for this sort of debug message. When you leave someone else will be responsible for dealing with the application and they shouldn't have to change the code because of that. > $confirm_debug_run = <>; Besides what I said above, <> by itself is magic, if there are any items in @ARGV, it will open them and try to read from them. If you are trying to interact with a human you should say <STDIN>. > my $present_date = DateTime->now(time_zone => 'America/New_York'); You have a config file, why isn't the timezone in it? Or better yet take it from the system the code is running on. > my $present_month_name = $present_date->month_abbr(); > my $present_month_num = $present_date->month; > my $present_year = $present_date->year; > my $present_date_string = "$present_month_name $present_year"; $present_month_num is not used and should be removed. Also, consider changing the code to my $present_date_string = $present_date->month_abbr . " " . $present_date->year; to get rid of the useless temporary variables. > my $eagleuser = EagleUser->new(); > my @userlist = $eagleuser->get_inf_user_list(0); > undef $eagleuser; The variable $eagleuser has too large a scope. You can tell this because you are bothering to undef it. Also, the variable servers no purpose. You have two good choices. The safest thing is to say my @userlist = do { my $eagleuser = EagleUser->new or die "could not create EagleUser object"; $eagleuser->get_inf_user_list(0); }; But if you are certain that EagleUser will never fail to create an object (or if it dies on its own if it fails to create an object) you can say my @userlist = EagleUser->new->get_inf_user_list(0); and avoid the temporary variable completely. > my (@users_over, @users_under, @users_grace) = (); Arrays start empty, so the assignment of an empty list achieves nothing. > my ($users_over_count, $users_under_count, $users_grace_count, $user_count) = > 0; All of these values can be derived from the arrays, so they are useless. And even worse, you only increment them, you never use their values for anything. Get rid of them. > my $users_to_email = {}; This is an odd choice, why not just use a hash? my %users_to_email; > foreach (@userlist) { > > my $username = $_; This is a very verbose and wasteful way of saying for my $username (@userlist) { > my $prefix = substr($username, 0); This variable is never used, it should be removed. > my $user = EagleUser->new(); Unless this class dies when it can't create an object, you should be saying my $user = EagleUser->new or die "could not create an EagleUser object"; > my $hours_left = 0; This variable is not used in this scope. It should not be declared here and definitely should not be set to zero. > foreach (1..5) { This looks like a magical constant. I distrust those. You might want to add a comment explaining why you are running the code five times. You also are using the value in the body of the loop, so it would be a good idea to give it a descriptive name. for my $plan_num (1 .. 5) { > my $plan_name_string = "Plan".$_."name"; > my $plan_hours_string = "Plan".$_."hours"; > my $plan_over_string = "Plan".$_."over_rate"; > my $plan_status_string = "Plan".$_."status"; The name string is not descriptive of the contents, a better name would be key. Use interpolation, not the concatenation operator: my $plan_name_key = "Plan${plan_num}name"; my $plan_hours_key = "Plan${plan_num}hours"; my $plan_over_key = "Plan${plan_num}over_rate"; my $plan_status_key = "Plan${plan_num}status"; Also, when you name a set of variables with the same prefix it is generally a sign that you really want a hash. A hash would also be easier to initialize: my %plan_key = map { $_ => "Plan{$plan_num}$_" } qw/name hours over_rate status/; > my $plan_name = $user->{$plan_name_string}; > my $plan_hours = $user->{$plan_hours_string}; > my $plan_over = $user->{$plan_over_string}; > my $plan_status = $user->{$plan_status_string}; Similarly, this looks like another unrolled hash: my %plan = @{$user}{values %plan_keys}; > if (($plan_name and $plan_name =~ /[Pp]lan[BbCcDd]/) and ($plan_hours and > $plan_hours > 0) and $plan_over > 0 and $plan_status ne 'delete') { Wow, that is a long if statement, it is nice if you break statements like this up into more digestible chunks. And you need to use the defined operator rather than just testing the value of the item. Also, the parentheses are unnecessary; (a and b) and (c and d) is the same as a and b and c and d. You would need parentheses if you threw an or in there, but there aren't any, so there is not point in cluttering up the expression with them: if ( defined $plan{name} and $plan{name} =~ /[Pp]lan[BbCcDd]/ and defined $plan{hours} and $plan{hours} > 0 and $plan{over} > 0 and $plan{status} ne 'delete' ) { Furthermore, the rest of the code in this loop lives inside this if statement. This means you have an unnecessary level of indentation. I prefer to say things like next unless defined $plan{name} and $plan{name} =~ /[Pp]lan[BbCcDd]/ and defined $plan{hours} and $plan{hours} > 0 and $plan{over} > 0 and $plan{status} ne 'delete'; which saves me a level of indentation and lets the reader know that they don't need to worry about this conditional having another branch later on. > my $hours_lookup = RadiusMgmt->new(); Again, unless your classes die on their own when they run into trouble, you should be checking to see if a valid object came back: my $hours_lookup = RadiusMgmt->new or die "could not create a RadiusMgmt object"; If this class has an error variable or method, you should use or call it and add the message to the die. > my $hours_used = ($hours_lookup->get_month_hours($dbh, $username, > $search_date, 'dialup')); Why are you creating a list here? Just say my $hours_used = $hours_lookup->get_month_hours($dbh, $username, $search_date, 'dialup'); > undef $hours_lookup; Again, this is a sign that you have done something wrong. $hours_lookup should either not exist, or it should be restricted to a smaller scope. You can use the same techniques from the EagleUser comment above. > if ($hours_used < $plan_hours) { > $hours_left = ($plan_hours - $hours_used); > push (@users_under, "$username $plan_name under by $hours_left > hours\n"); > $users_under_count++; > $user_count++; > # grace > > } elsif (($hours_used - $plan_hours) <= 2) { > push (@users_grace, "$username $plan_name was within 2 hour grace\n"); > $users_grace_count++; > $user_count++; > # over > > } elsif ($hours_used > $plan_hours) { > $hours_left = ($hours_used - $plan_hours); > push (@users_over, "$username $plan_name over by $hours_left hours\n"); > $users_to_email->{$username}->{$plan_name_string} = $hours_left; > $users_over_count++; > $user_count++; > } You are recalculating some version of $hours_used - $plan_hours several times. This is wasteful. Also, the last elsif is pointless, it must be true based on the other if and elsif statements, so it should just be an else. I would write it like this my $hours_left = $plan_hours - $hours_used; if ($hours_used > 0) { push @users_under, "$username $plan{name} under by $hours_left hours\n"; } elsif ($hours_left >= -2) { push @users_grace, "$username $plan{name} was within 2 hour grace\n"; } else { push @users_over, "$username $plan{name} over by $hours_left hours\n"; $users_to_email{$username}{$plan{name}} = $hours_left; } > undef $user; This is unnecessary, $user will be destroyed when it goes out of scope. > #while ( my ($username, $planname) = each (%{$users_to_email})) { > # while ( my ($key, $value) = each (%{$planname})) { > # print "$username :: $key => $value\n"; > # } > #} Don't leave commented chunks of code in your program. That is what a source control program is for. You are using a source control program right? > my $user = EagleUser->new; see above > $user->build_inf_user($username); You already had this. Unless this object takes up a large amount of memory, it is wasteful to keep creating it. You might want to save the object in the hash. > my $plan_hours_allowed_string = $plan.'hours'; > my $plan_hours_allowed = $user->{$plan_hours_allowed_string}; Why not just say my $plan_hours_allowed = $user->{"${plan}hours"}; > my $client_email_overhours_tpl = Template->new( ABSOLUTE => 1); > $client_email_overhours_tpl->process("$template_dir/$client_invoice_tpl", > \%client_msg_template_data) or warn $client_email_overhours_tpl->error; If this is in fact Template::Toolkit, then you are supposed to call my $client_email_overhours_tpl = Template->new( ABSOLUTE => 1); once outside the loop and reuse it by calling the process each time through the loop. > undef %client_msg_template_data; Like the other cases, this is unnecessary, it will go out of scope and be destroyed in a few lines. > exit(); That looks like a line left over from some debugging you were doing. It will cause the program to stop after it sends the very first email. I suggest when you are dropping in debug code you either put it in an if statement: exit if $debug; or you tag it with a comment so you can find it later easily: #FIXME: this is a debug statement exit; > if ($email_accounting) { Again, don't leave your reader in suspense. The rest of the code for this program is in this if statement. It should really be reworded to let the reader know that right now (rather than forcing them to read the rest of code to find out if there is an else statement or more code): exit unless $email_accounting; -- Chas. Owens wonkden.net The most important skill a programmer can have is the ability to read. -- To unsubscribe, e-mail: beginners-unsubscr...@perl.org For additional commands, e-mail: beginners-h...@perl.org http://learn.perl.org/
