How to clean up this ugly code?

[blaine]

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

 

[Joost Diepenmaat]

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.

 

[Ben Morrow]

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

 

[Dave Weaver]

On Thu, 22 Nov 2007 15:21:45 -0800 (PST),

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();

 

[comp.llang.perl.moderated]

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

 

[blaine]

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;

 

[comp.llang.perl.moderated]

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.