http://www.perlmonks.org?node_id=417727

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

Hello!

I'm still practise my skill in Perl. I walk slowly, but it's very nice travel by Camel's track.
So, I need some advices about style code.

Look please at this:
To try OO programing in Perl I've made module to calculate how many bills and coins I need to pay for example $133 = (100+20+10+2+1).
It's propably quite helpful for employers.
He can't get 5x$100 from bank for 6 workers ($135 + $80 + $66 + $150 + $69= $500),
but he can do this:
#!/usr/bin/perl use warnings; use strict; use payment; my @my_bills = qw (200 100 50 20 10 5 2 1); my @my_payments = qw{135 80 66 150 69}; my $mp = payment->new(); $mp->change(@my_bills); foreach (@my_payments) { $mp->add($_); } my %result = $mp->get(); foreach ( sort { $b <=> $a } keys(%result) ) { print "$_ \t: $result{$_}\n"; }
and this is code of payment.pm
package payment; use warnings; use strict; sub new { my $class = shift; my $self = {}; $self->{units} = undef; $self->{bills} = undef; bless $self, $class; } sub change { my $self = shift; @{ $self->{units} } = @_; %{ $self->{bills} } = map { $_ => 0 } @_; } sub add { my $self = shift; my @units = @{ $self->{units} }; my %pieces = %{ $self->{bills} }; my $temp = shift; my $unit; my $i = 0; while ($temp) { $unit = $units[$i]; while ( $temp >= $unit ) { $temp -= $unit; $pieces{$unit} += 1; } last if $i == (@units); $unit = $units[ ++$i ]; } %{ $self->{bills} } = %pieces; } sub get { my $self = shift; return %{ $self->{bills}}; } 1;
So, what do you thing about this? My style is good? Have you any advices for me? If it's good, I'm going to add some features into my class. ;-)

Thanks for your's advices and time.
Regards
Uksza

Replies are listed 'Best First'.
Re: Payment changer
by trammell (Priest) on Dec 28, 2004 at 16:01 UTC
    The code looks fine to me, but where's the documentation? You might want to add some POD or at least a few comments before adding new features.
Re: Payment changer
by Forsaken (Friar) on Dec 29, 2004 at 00:29 UTC
    1 small comment i'd like to make is on the following part:
    sub add { my $self = shift; my @units = @{ $self->{units} }; my %pieces = %{ $self->{bills} }; my $temp = shift;
    I personally use the guideline that it's best to clean out @_ and assign all the values that i need before doing anything else. 9 out of 10 times this is completely unnecessary, but consistently doing it does prevent me from leaving things in @_ only to have them changed later, yielding unpredictable results. so my advice would be to start off with my ($self, $temp) = @_; and then to proceed with the remainder of the sub.