How to clean up this ugly code?

Hey,

I have this ugly code below that I would like to get rid of everything
in the switch statement. My preference would be to have something
simple that just takes and id and calls that sub routine number.

I suppose I could use and eval like
eval( "\$self->DisplayPage$subpage_id");

but was wondering if someone can think of something better, as then if
there is an error it's trapped in eval..

Code I'd like to clean is below...

SWITCH: for ($subpage_id)
{
if (/^1$/) {$self->DisplayPage; last SWITCH;}
elsif (/^2$/) {$self->DisplayPage2; last SWITCH;}
elsif (/^3$/) {$self->DisplayPage3; last SWITCH;}
elsif (/^4$/) {$self->DisplayPage4; last SWITCH;}
elsif (/^5$/) {$self->DisplayPage5; last SWITCH;}
elsif (/^6$/) {$self->DisplayPage6; last SWITCH;}
elsif (/^7$/) {$self->DisplayPage7; last SWITCH;}
elsif (/^8$/) {$self->DisplayPage8; last SWITCH;}
elsif (/^9$/) {$self->DisplayPage9; last SWITCH;}
}

Thanks,
Blaine

Re: How to clean up this ugly code?

my $method = "DisplayPage$subpage_id";
$self->$method;

is strict "clean" but you might want to check that $subpage_id is an int
in the range you expect.

Joost.

Re: How to clean up this ugly code?

Quoth "blaine@worldweb.com" :
>
> I have this ugly code below that I would like to get rid of everything
> in the switch statement. My preference would be to have something
> simple that just takes and id and calls that sub routine number.
>
> I suppose I could use and eval like
> eval( "\$self->DisplayPage$subpage_id");
>
> but was wondering if someone can think of something better, as then if
> there is an error it's trapped in eval..
>
> Code I'd like to clean is below...
>
> SWITCH: for ($subpage_id)
> {
> if (/^1$/) {$self->DisplayPage; last SWITCH;}
> elsif (/^2$/) {$self->DisplayPage2; last SWITCH;}

As has already been pointed out, you can use a variable as a method
name under 'use strict'. However, the first question is why you have
these methods at all. It would be much better to have one method
->DisplayPage that took the page number as a parameter. If the pages
really benefit from being in separate subs then you can do something
like

sub DisplayPage {
my $page = shift;
(
sub {
# page 1
},
sub {
# page 2
},
)[$page - 1]->(@_);
}

Ben

Re: How to clean up this ugly code?

On Thu, 22 Nov 2007 15:21:45 -0800 (PST),
blaine@worldweb.com wrote:
>
> Code I'd like to clean is below...
>
> SWITCH: for ($subpage_id)
> {
> if (/^1$/) {$self->DisplayPage; last SWITCH;}
> elsif (/^2$/) {$self->DisplayPage2; last SWITCH;}
> elsif (/^3$/) {$self->DisplayPage3; last SWITCH;}
> elsif (/^4$/) {$self->DisplayPage4; last SWITCH;}
> elsif (/^5$/) {$self->DisplayPage5; last SWITCH;}
> elsif (/^6$/) {$self->DisplayPage6; last SWITCH;}
> elsif (/^7$/) {$self->DisplayPage7; last SWITCH;}
> elsif (/^8$/) {$self->DisplayPage8; last SWITCH;}
> elsif (/^9$/) {$self->DisplayPage9; last SWITCH;}
> }

my @dispatch = (
\&DisplayPage,
\&DisplayPage2,
\&DisplayPage3,
# ...
);

my $method = $dispatch[ $subpage_id - 1 ]
or die "Page '$subpage_id' doesn't exist";

$self->$method();

Re: How to clean up this ugly code?

On Nov 22, 3:21 pm, "bla...@worldweb.com" wrote:
> Hey,
>
> I have this ugly code below that I would like to get rid of everything
> in the switch statement. My preference would be to have something
> simple that just takes and id and calls that sub routine number.
>
> I suppose I could use and eval like
> eval( "\$self->DisplayPage$subpage_id");
>
> but was wondering if someone can think of something better, as then if
> there is an error it's trapped in eval..
>
> Code I'd like to clean is below...
>
> SWITCH: for ($subpage_id)
> {
> if (/^1$/) {$self->DisplayPage; last SWITCH;}
> elsif (/^2$/) {$self->DisplayPage2; last SWITCH;}
> elsif (/^3$/) {$self->DisplayPage3; last SWITCH;}
> elsif (/^4$/) {$self->DisplayPage4; last SWITCH;}
> elsif (/^5$/) {$self->DisplayPage5; last SWITCH;}
> elsif (/^6$/) {$self->DisplayPage6; last SWITCH;}
> elsif (/^7$/) {$self->DisplayPage7; last SWITCH;}
> elsif (/^8$/) {$self->DisplayPage8; last SWITCH;}
> elsif (/^9$/) {$self->DisplayPage9; last SWITCH;}
> }

Untested:

my %methods = map
{ ( qr/^$_/, 'DisplayPage' . $_ ) }
1..9;

SWITCH: {
while ( my ($re, $method ) = each %methods ) {
$self->$method if $subpage_id =~ /$re/;
last SWITCH;
}
die "no method for $subpage_id..\n";
}

--
Charles DeRykus

Re: How to clean up this ugly code?

Wow thanks for all your input! Everyone has some great ideas.

I'm going to use Joost's idea, seems really nice!

Thanks again!
Blaine

my $method = "DisplayPage$subpage_id";
$self->$method;

On Nov 22, 4:21 pm, "bla...@worldweb.com" wrote:
> Hey,
>
> I have this ugly code below that I would like to get rid of everything
> in the switch statement. My preference would be to have something
> simple that just takes and id and calls that sub routine number.
>
> I suppose I could use and eval like
> eval( "\$self->DisplayPage$subpage_id");
>
> but was wondering if someone can think of something better, as then if
> there is an error it's trapped in eval..
>
> Code I'd like to clean is below...
>
> SWITCH: for ($subpage_id)
> {
> if (/^1$/) {$self->DisplayPage; last SWITCH;}
> elsif (/^2$/) {$self->DisplayPage2; last SWITCH;}
> elsif (/^3$/) {$self->DisplayPage3; last SWITCH;}
> elsif (/^4$/) {$self->DisplayPage4; last SWITCH;}
> elsif (/^5$/) {$self->DisplayPage5; last SWITCH;}
> elsif (/^6$/) {$self->DisplayPage6; last SWITCH;}
> elsif (/^7$/) {$self->DisplayPage7; last SWITCH;}
> elsif (/^8$/) {$self->DisplayPage8; last SWITCH;}
> elsif (/^9$/) {$self->DisplayPage9; last SWITCH;}
> }
>
> Thanks,
> Blaine

Re: How to clean up this ugly code?

Ben Morrow wrote:
> However, the first question is why you have
> these methods at all. It would be much better to have one method
> ->DisplayPage that took the page number as a parameter.

Good Question, and unanswered as yet.

All the other "direct" answers to OP's original
post are interesting pieces of Perl, but
don't address the largest flaw in the
overall implementation.

BugBear

Re: How to clean up this ugly code?

On Nov 23, 8:36 am, bugbear wrote:
> Ben Morrow wrote:
> > However, the first question is why you have
> > these methods at all. It would be much better to have one method
> > ->DisplayPage that took the page number as a parameter.
>
> Good Question, and unanswered as yet.
>
> All the other "direct" answers to OP's original
> post are interesting pieces of Perl, but
> don't address the largest flaw in the
> overall implementation.
>

With the trivial code shown, that's certainly true but since that
question is still not "answered", there may be some wiggle room...

Even if the methods could be housed together, separating them out
might be preferable due
to auto-generation of the methods, or complexity, or sheer
combinatorial bloat.
Or maybe this is a case of "easy on the eyes" winning out over over
efficiency.

--
Charles DeRykus