Otherwise, it all looks fine. You've got a lot of the idioms I would use in an OO situation.

A few ideas to bring (and boggle) you further:

1. Use closures to generate the accessors in Expense.

   sub new {
       # Stuff up here
       foreach my $attrib (keys %$self) {
           my $conv_attrib = $attrib;
           $conv_attrib =~ s/^_(\w+)$/lc $1/e;
   
           no strict 'refs';
           *{__PACKAGE__ . "::$conv_attrib"} = gen_closure($attrib);
       }
       return $self;
   }
   
   sub gen_closure {
       my $attrib = shift;
   
       return sub {
          my $self = shift;
          if (@_) { $self->{$attrib} = shift; }
          return $self->{$attrib};
      }
   }
   [download]

   That is much easier to maintain when you start hitting 20 or 30 attributes in your objects.
2. Look at separating your database logic from your business logic. This is something we're hitting where I work right now. It'll sound crazy, but if this is going to scale, you will need to do that. Maybe something along the lines of DB::Expense so that if your database schema changes (which it will), you aren't left scrambling to fix your beautiful OO structures.

Good luck!

------
We are the carpenters and bricklayers of the Information Age.

Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

You could conserve resources by generating constructors at "compile" time rather than every time the constructor (new) is called. Class::MethodMaker could do this for you:

use Class::MethodMaker
  get_set => [ qw( accessor1 accessor2 ) ];
[download]

Or you could update 'new' above to check to see if the accessor exists before re-building it.

Fine. Change the = to ||= and it's ok.

------
We are the carpenters and bricklayers of the Information Age.

Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

Damn, that closure code threw me for a loop at first. It took two or three times before I fully understood what was going on. I haven't really had the need or desire to play with typeglobs and creating subroutines on the fly, but maybe I need to take a look at it, seems like it could save me some work if I learn a few tricks...

sub new {
   my $class = shift;
   my $id    = shift;
   my $self  = {
      _ID        => $id,
      _FINALIZED => 0,
      _EXPENSES  => [],
      _TOTAL     => undef,
      _COUNT     => undef,
   };
   return bless $self, $class;
}
[download]

• in ExpenseReport::add_expense() -

  $newexp->{_ID} = $#expenses + 1;
  [download]

  seems more intuitive as

  $newexp->{_ID} = scalar @expenses;
  [download]

• in ExpenseReport::save_file - get rid of the temp variable and just use

  local $Data::Dumper::Indent = 3;
  [download]

  You are doing that in ExpenseReport::finalize, so i suspect this code was overlooked.
• ExpenseReport::load_file() - try this instead

  # open file
  my $file = do {local $/; <REPORT>};
  [download]

• I notice that you connect, disconnect, and reconnect several times to the database - this may be desired behavior, but consider opening a connection and saving it as an attribute of the class, or as another class.
• Inside ExpenseReport::finalized() and all of Expense's methods - change those one line if's to:

  $self->{$hiddenkey} = shift if @_;
  [download]

Over all, very good work. And good luck with the next step. :)

jeffa

L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)

Maybe I just don't understand WHY the ref($cargocult) idea is so bad. To me, it makes perfect sense... If someone calls the constructor on an instance rather than the class, it forces it to act as if it were called on the class. Isn't this "correct" in most cases?

I tried putting the line you tried and then calling my constructor on an already running instance, and it does return an object, but one blessed incorrectly(read: differently) for my purposes. When I try and invoke any methods on this object that is blessed into "Expense=HASH(0x123456)" and not "Expense" it gives me an error.

So, basically, my question is this... Can you please give me a real world scenario or two where I would want to call $object->new and not CLASS->new, and then an example of how you use that new returned object's methods, since it is not blessed into the correct class? Here's the code I used to come to these conclusions:

Read more... (2 kB)

Calling an instance constructor? Yuck! If you want to clone an object, use the right tool, such as Clone:

use strict;
use Clone qw(clone);

my $foo = foo->new(5);
my $bar = clone($foo);

print $bar->id(), "\n";

package foo;
use strict;

sub new {
   my ($class,$id) = @_;
   my $self = {
      id => $id || 42,
   };
   return bless $self,$class;
}

sub id {
   my ($self,$id) = @_;
   return $self->{id} unless $id;
   $self->{id} = $id;
}
[download]

or provide your own clone() method for the class. I really recommend that you read TheDamian's excellent book, Object Oriented Perl, by the way.

jeffa

L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)

Maybe I just don't understand WHY the ref($cargocult) idea is so bad. To me, it makes perfect sense... If someone calls the constructor on an instance rather than the class, it forces it to act as if it were called on the class. Isn't this "correct" in most cases?

new is new and clone is clone. Conviently merging the two into one and differentiating by the call context feels like an icky version of polymorphism.

btw: Great node, but I gotta tell ya- there are days when I wish I was bagging groceries rather than coding/debugging/supporting. :)

Here are some nodes w/ some really good explanations:

Ovid's (Ovid - minor code nits) Re: Adding autoloaded methods to symbol table with using strict refs
merlyn's ref($proto) - just say no!

It is bad because while you can have the constructor work fine if someone is confused about the difference between a class and an instance of the class, you cannot perform the same trick for any other non-trivial methods. See Re (tilly) 2: Paradigm Shift - Dual Use Constructors for more detailed discussion of that point.

sub add_expense { # takes Expense object as argument and adds it to _E
+XPENSES array
    my $self = shift;
    my $newexp = shift;
    my @expenses = @{$self->{_EXPENSES}};
    $newexp->{_ID} = $#expenses + 1;
    push(@expenses,$newexp);
    $self->{_EXPENSES} = \@expenses;
}
[download]

Where are the problems with this code?

We have a couple. The first is simply syntax:

    my $self = shift;
    my $newexp = shift;
[download]

Simple, clean, one look gives you all the incoming sub arguments, expands easily:

my ($self, $newexp) = @_;
[download]

my @expenses = @{$self->{_EXPENSES}};
[download]

Why would you do this? with this one operation, you have just duplicated the entire array for no real gain. Certainly if you were going to reference the array many times it *might* be worth it, or if you were likely to bail part-way through your modifications and want the array to remain as it was until you make your final change, but in this instance its pure inefficiency to no value. If you don't feel comfortable referencing @{$self->{_EXPENSES}} twice, then do my $expenses = $self->{_EXPENSES} instead and use the reference.

The entire ID concept gives me shivers. This is the kind of code people wrote when they didn't have associative arrays, with "rehash" methods to compact down a data structure that had elements tagged for deletion (undef) etc. Its simply not healthy, your interface as it stands allows you to add a single expense twice (BAD, _ID would be reset and the Expense object would be confused), requires compacting, does not guarrantee unique ids (create Expense, keep reference, add, delete, rehash, add new Expense, doh, has same ID as old reference), and is basically hard work.

Instead, use an associative array keyed by the reference. If you need to maintain order you can use one of the more advanced structs on CPAN to do that at the same time, but the advantages of an associative keyed by reference are that you never have duplicate IDs, IDs don't need to be explicitly tracked, adding a single expense multiple times is fine and you never need to rehash.

My final code?

sub add_expense { # takes Expense object as argument and adds it to _E
+XPENSES array
    my ($self, $newexp) = shift;
    $self->{_EXPENSES}{$newexp} = $newexp;
}
[download]

:)

my ($self, $newexp) = @_;

I hate this syntax construction. Absolutely hate it. "Why?" you might ask?

It's because $self isn't passed in to the function by the caller, it's passed in by the system. In my code, I like to differentiate that. This also allows me to go ahead and see my methods vs. any helper functions I might have defined.

sub blah {
    my $self = shift;
    my ($other, $vars) = @_;
}
[download]

You get the expandability as well as the identification that it's a method call, all for the price of a cup of coffee.

------
We are the carpenters and bricklayers of the Information Age.

Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

Fair enough call. I hate your way, but I like it better than 5 lines of = shift :)

I'd love to, but I live in Miami, and the Miami.pm site was last updated on March 11, 1999. I don't think there will be much luck kicking the dust off that one.

I'm not so sure about that... Just because they dont update their homepage doesnt mean they dont exists. Try writing to the mailing list, setup an IRC channel. Figure out who is in change of updating the homepage, maybe he left pm and didnt give permission to anyone else to update and noone has been brave enough to take over...

ps. sorry about all the double negations ;)

T I M T O W T D I

Very nice - and far better than my first (or indeed current) attempts at OO code. One query - Is it a good idea to have a get_total function which does the additon. My first instinct would be to make add_expenses and delete_expenses keep the _TOTAL up to date at all times. Any opinons on this?

I thought about that, but I figured it would be less expensive to do the math only when I actually need the number, rather than every time I would have changed. I figure if I'm going to access the data through a method, I might as well have it generate it on the fly and not when I don't need it.