Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Perl > Perl Misc > How to clean up this ugly code?

Reply
Thread Tools

How to clean up this ugly code?

 
 
blaine@worldweb.com
Guest
Posts: n/a
 
      11-22-2007
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
 
Reply With Quote
 
 
 
 
Joost Diepenmaat
Guest
Posts: n/a
 
      11-22-2007
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.

 
Reply With Quote
 
 
 
 
Ben Morrow
Guest
Posts: n/a
 
      11-23-2007

Quoth "(E-Mail Removed)" <(E-Mail Removed)>:
>
> 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

 
Reply With Quote
 
Dave Weaver
Guest
Posts: n/a
 
      11-23-2007
On Thu, 22 Nov 2007 15:21:45 -0800 (PST),
http://www.velocityreviews.com/forums/(E-Mail Removed) <(E-Mail Removed)> 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();

 
Reply With Quote
 
comp.llang.perl.moderated
Guest
Posts: n/a
 
      11-23-2007
On Nov 22, 3:21 pm, "(E-Mail Removed)" <(E-Mail Removed)> 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
 
Reply With Quote
 
blaine@worldweb.com
Guest
Posts: n/a
 
      11-23-2007
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, "(E-Mail Removed)" <(E-Mail Removed)> 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


 
Reply With Quote
 
comp.llang.perl.moderated
Guest
Posts: n/a
 
      11-23-2007
On Nov 23, 8:36 am, bugbear <bugbear@trim_papermule.co.uk_trim> 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
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Re: How include a large array? Edward A. Falk C Programming 1 04-04-2013 08:07 PM
Visual C++'s :"clean solution" doesn't clean the solution? plenty900@yahoo.com C++ 8 05-31-2008 04:57 PM
How do I clean a virus within an inbox or just clean only that infectedattachment or LOCATE AND delete just that attachment ? Vinayak Firefox 1 08-14-2006 06:19 PM
Clean up my Hard drive before selling it. Clean Registry? what else? baaab Computer Support 5 05-10-2005 08:30 AM
Mozilla Firebird 0.7 has ugly font Nick de Graeve Firefox 1 02-03-2004 05:56 PM



Advertisments