in reply to Is there a better way to do this?

markdibley:

A couple things:

Taking those into account, I'd restructure your code into something more like the following (untested, yadda yadda):

#!/usr/bin/perl -w use strict; use Data::Dumper; use sam; use tes; # Create a sample my $sample = new sam("organite"); # Create a new test, name it, add it to the sample and increase it's s +tatus a couple of times my $test1 = new tes("first", $sample); $test1->status("new", "me"); $test1->status("stage one", "me"); # Create ANOTHER new test, name it, add it to the sample and increase +it's status a couple of times my $test2 = new tes("second", $sample); $test2->status("new", "me"); $test2->status("stage one", "me"); # change test2 status $test2->status("stage four", "me"); # change test1 status, but not as high as test2 $test1->status("stage two", "me"); printf("\nSample History (%s)\n%s\n", $sample->sample_id(), $sample->s +tatus_history()); printf("\ntest1 is currently at: %s\n",$test1->current_status()); printf("\ntest2 is currently at: %s\n",$test2->current_status()); printf("\nsample is currently at: %s\n\n",$sample->current_status());

Package sam

package sam; use strict; use Data::Dumper; use status; sub new{ my($class, $name) = @_; my $this = { 'sample_id'=>$name, 'test_list'=>[], 'status'=>[], } ; bless($this, $class); return $this; } sub sample_id{ return $_[0]->{'sample_id'} } sub add_test{ my($this, $test) = @_; # Add test to sample push @{$this->{'test_list'}}, $test; } sub add_status{ my($this, $test) = @_; my $h = {}; $h->{'ref'} = ${$test->{'status'}}[(scalar(@{$test->{'status'}}) - + 1)]; $h->{'src'} = $test; push @{$this->{'status'}}, $h; } sub status_history{ my($this) = @_; my $history = ""; foreach my $entry(@{$this->{'status'}}){ $history .= sprintf("%s\t%s\t%s\t%s\n", $entry->{src}->test_id +(), $entry->{'ref'}->status(), $entry->{'ref'}->time(), $entry->{'ref +'}->owner()); } return $history; } sub current_status{ my($this) = @_; my $top; foreach my $entry(@{$this->{'status'}}){ $top = $entry if(! defined $top || $entry->{'ref'}->status_cod +e() > $top->{'ref'}->status_code()); } return $top->{'ref'}->status; } 1;

Package tes

package tes; use strict; use Data::Dumper; use status; sub new{ my($class, $name, $sample, $data) = @_; my $this = { 'sample'=>$sample, 'test_id'=>$name, 'status'=>[], } ; bless($this, $class); $sample->add_test($this); return $this; } sub test_id{ my($this) = @_; return $this->{'test_id'} } sub status{ my($this, $status, $owner) = @_; if(! defined $status){ return pop @{$this->{'status'}}; } else{ my $stat = new status($status, $owner); push @{$this->{'status'}}, $stat; $this->{'sample'}->add_status($this); } return 1; } sub current_status{ my($this) = @_; my $status = $this->status(); return $status->status(); } 1;

Package status

package status; use strict; use Data::Dumper; my $order = {"new"=>1,"stage one"=>2,"stage two"=>3,"stage four"=>4,"e +nd"=>5}; sub new{ my($class, $status, $owner) = @_; my $this = { 'status'=>$status, 'time'=>time(), 'owner'=>$owner}; bless($this, $class); return $this; } sub status{ my($this, $status) = @_; if(defined $status){ $this->{'status'} = $status; } return $this->{'status'}; } sub status_code{ my($this, $status) = @_; if (exists $order->{$this->{status}}) { return $order->{$this->{'status'}}; } else{ return undef; } } sub time{ my($this) = @_; return $this->{'time'}; } sub owner{ my($this) = @_; return $this->{'owner'}; } 1;

We could do a bit more factoring, etc., but this'll give you an idea of the direction I'd go.

...roboticus

When your only tool is a hammer, all problems look like your thumb.

Replies are listed 'Best First'.
Re^2: Is there a better way to do this?
by markdibley (Sexton) on Dec 15, 2010 at 16:20 UTC
    Thanks!

    I understand and agree with both your points. I know I write my code quiet loose, usually because I know that the people I code for come up with the most annoying exceptions several months down the line :-)

    I was most worried about sam::add_status so I'm glad that has passed inspection so far. I just need to improve everything else.

    Thanks again for everyone who has replied.

      If you are interested in how good your code "looks", install Perl::Critic and run the perlcritic utility. Note that perlcritic knows many rules, and there are several rules that can be ignored or broken. After all, it is a tool to compare perl source with the recommendations in PBP. Perl itself doesn't really care how your code "looks", but your future self perhaps will, because he has to maintain what you write now.

      In default mode, perlcritic finds only one thing:

      # perlcritic . ./877252.pl source OK ./sam.pm source OK ./status.pm: "return" statement with explicit "undef" at line 45, colu +mn 3. See page 199 of PBP. (Severity: 5) ./tes.pm source OK

      In "brutal" mode (perlcritic --brutal .), perlcritic spits out 96 lines, each with a major or minor problem. Missing version control system markers, indirect object syntax, useless interpolation, whitespace at end of line, and of course the all-lowercase module names, to name some.

      All-lowercase module names are reserved for pragmatic modules, you should perhaps change your module names to start with an uppercase letter. Also, when your project grows, better have a separate "namespace" for it. If you have no better idea, start with your name or your company's name, followed by the project name. E.g. BigCorp::Frobnicator::Parser, BigCorp::Frobnicator::Generator, BigCorp::Frobnicator::Logger.

      Alexander

      --
      Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
        Thanks again. looks like I will be using perlcritic a lot from now on. I'm always very concious that someone will eventually have to take over my code (I hope!)

        As for the module names, I confess that was just laziness on my part just for this experiment/post. I didn't want them to get mixed up with Sample and Test which are the existing modules so I changed the case - and then stripped off the ends of the names too for the hell of it.

        I'm surprised it doesn't complain about the lack of commenting too.

        Thanks again