Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

My first stab at OO perl...

by Theseus (Pilgrim)
on Jul 16, 2002 at 15:15 UTC ( #182105=perlquestion: print w/ replies, xml ) Need Help??
Theseus has asked for the wisdom of the Perl Monks concerning the following question:

I've been coding perl for a few years now(though only as my day job for the past few months), and about a week ago I decided to buckle down and finally learn object orientation, since that being my weak spot always hurt me when I use modules. I think understanding the theory behind how they work has helped me out a great deal.

In any case, I figured I'd put my first two classes up here for review. I've never met anyone else in person who knew how to code perl, so this is really my first time putting anything I've written that wasn't just fooling around up for review. I'm looking for constructive criticism, as long as it isn't along the lines of "stop programming perl and get a job bagging groceries." Here's the text of ExpenseReport.pm and Expense.pm:

package ExpenseReport; sub new { my $cargocult = shift; my $class = ref($cargocult) || $cargocult; my $id = shift; my $self = {}; $self->{_ID} = $id | undef; $self->{_FINALIZED} = 0; $self->{_EXPENSES} = []; # Array of Expense objects $self->{_TOTAL} = undef; # Total sum of all totals of Expense obje +cts in _EXPENSES $self->{_COUNT} = undef; # $#{$self->{_EXPENSES}} + 1 bless $self, $class; } sub get_total { #calculates total of all expenses in the report and re +turns the result my $self = shift; my $total; foreach my $exp (@{$self->{_EXPENSES}}) { $total += $exp->amount; } $self->{_TOTAL} = $total; return $total; } sub expenses { # returns array of expense objects in array context or +a reference to the _EXPENSES array in scalar context my $self = shift; my @expenses = @{$self->{_EXPENSES}}; wantarray ? return @expenses : return $self->{_EXPENSES}; } 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; } sub delete_expense { # should be rewritten to use splice when I get a +chance my $self = shift; my $id = shift; delete ${$self->{_EXPENSES}}[$id]; $self->rehash; } sub save_file { my $self = shift; my $filename = shift; my $temp = $Data::Dumper::Indent; $Data::Dumper::Indent = 3; open(OUT,">$filename"); print OUT Data::Dumper->Dump( [ $self ] , [ 'report' ] ); close(OUT); $Data::Dumper::Indent = $temp; } sub load_file { my $self = shift; my $filename = shift; my $report; #print $filename,"\n"; { local $/ = undef; open(REPORT,"$filename") or &error("Couldn't load expense repo +rt... $!"); my $file = <REPORT>; close(REPORT); eval($file) or print "couldnt eval -- $!"; return $report; } } sub load_db { my $self = shift; my $id = shift; my $dbh = DBI->connect('dbi:ODBC:expense',undef,undef, { AutoCommi +t => 1, RaiseError => 1, LongReadLen => 500000, LongTruncOk => 1 } ); my $hashref = $dbh->selectrow_hashref("SELECT * FROM requests WHER +E id = $id"); my $report; eval($$hashref{object}); return $report; } sub rehash { # compact array, refresh total my $self = shift; STARTCHECK: for $i (0 .. $#{$self->{_EXPENSES}}) { if (${$self->{_EXPENSES}}[$i] == undef) { splice(@{$self->{_EXPENSES}},$i,1); goto STARTCHECK; } } for $i (0 .. $#{$self->{_EXPENSES}}) { ${$self->{_EXPENSES}}[$i]->id($i); } $self->get_total; $self->{_COUNT} = $#{$self->{_EXPENSES}} + 1; } sub count { $self = shift; return $self->{_COUNT}; } sub finalize { my $self = shift; my $id = shift; my $dbh = DBI->connect('dbi:ODBC:expense', undef, undef, { AutoCom +mit => 1, RaiseError => 1, LongReadLen => 5000000, LongTruncOk => 1, +} ); $dbh->do("update requests set finalized = 1 where id = $id"); my $sql = "update requests set object = ? where id = $id"; my $sth = $dbh->prepare($sql); $sth->bind_param(1, $sql, DBI::SQL_LONGVARCHAR); $self->{_FINALIZED} = 1; { local $Data::Dumper::Indent = 0; $sth->execute(Data::Dumper->Dump( [ $self ] , [ 'report' ] )); } $dbh->disconnect; } sub finalized { my $self = shift; my $hiddenkey = "_FINALIZED"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; } #--------------------------------- #--------------------------------- package Expense; sub new { my $cargocult = shift; my $class = ref($cargocult) || $cargocult; my $self = {}; $self->{_DESCRIPTION} = undef; $self->{_PLACE} = undef; $self->{_AMOUNT} = undef; $self->{_ID} = undef; $self->{_DETAILS} = undef; bless $self, $class; } sub place { my $self = shift; my $hiddenkey = "_PLACE"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; } sub description { my $self = shift; my $hiddenkey = "_DESCRIPTION"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; } sub amount { my $self = shift; my $hiddenkey = "_AMOUNT"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; } sub id { my $self = shift; my $hiddenkey = "_ID"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; } sub details { my $self = shift; my $hiddenkey = "_DETAILS"; if (@_) { $self->{$hiddenkey} = shift; } return $self->{$hiddenkey}; }

Comment on My first stab at OO perl...
Download Code
Re: My first stab at OO perl...
by dragonchild (Archbishop) on Jul 16, 2002 at 15:33 UTC
    *laughs* ref($cargocult) || $cargocult; Very nice! :-)

    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}; } }
      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.

      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...
      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 ) ];
      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.

DUPLICATE - Re: My first stab at OO perl...
by NodeReaper (Curate) on Jul 16, 2002 at 15:51 UTC

    Reason: jeffa (delete) double post of 182110 (system hiccup?)

    For more information on this node visit: this

(jeffa) Re: My first stab at OO perl...
by jeffa (Chancellor) on Jul 16, 2002 at 15:54 UTC
    Looks like a good first stab to me. Biggest gripe i have is with the constructors, and it is not ironic that you would use a variable named $cargocult ;) - try this instead:
    sub new { my $class = shift; my $id = shift; my $self = { _ID => $id, _FINALIZED => 0, _EXPENSES => [], _TOTAL => undef, _COUNT => undef, }; return bless $self, $class; }
    Get rid of that 'ref (proto)' malarky and note that $id will already be undef if it is not specified. Other than that, everything seems OK - i do have a list of minor nitpicks though:
    • in ExpenseReport::add_expense() -
      $newexp->{_ID} = $#expenses + 1;
      seems more intuitive as
      $newexp->{_ID} = scalar @expenses;
    • in ExpenseReport::save_file - get rid of the temp variable and just use
      local $Data::Dumper::Indent = 3;
      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>};
    • 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 @_;
    Having said all of that, you should now take a look at CPAN modules like Class::Struct and Class::MethodMaker. I think these modules will help you design your classes by giving you an interface to work with. Also - use POD! This is a must for any class. Include a section with sample usage.

    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:

        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; }
        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.
Re: My first stab at OO perl...
by Cine (Friar) on Jul 16, 2002 at 23:36 UTC
    May I suggest that you involve yourself in a perl mongers club in your area... pm.org

    T I M T O W T D I
      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
Re: My first stab at OO perl...
by Anonymous Monk on Jul 17, 2002 at 00:46 UTC
    Not a comment on your code in particular although I'll use yours as an example:

    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; }

    Where are the problems with this code?

    We have a couple. The first is simply syntax:

    my $self = shift; my $newexp = shift;

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

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

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

    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; }

    :)

      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) = @_; }

      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 :)
Re: My first stab at OO perl...
by astaines (Curate) on Jul 17, 2002 at 16:01 UTC

    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?


    --
    Anthony Staines
      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.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://182105]
Approved by svad
Front-paged by djantzen
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (10)
As of 2014-10-02 14:06 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    What is your favourite meta-syntactic variable name?














    Results (61 votes), past polls