How to clean up this ugly code?

Discussion in 'Perl Misc' started by blaine@worldweb.com, Nov 22, 2007.

  1. Guest

    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
    , Nov 22, 2007
    #1
    1. Advertising

  2. 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.
    Joost Diepenmaat, Nov 22, 2007
    #2
    1. Advertising

  3. Ben Morrow Guest

    Quoth "" <>:
    >
    > 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
    Ben Morrow, Nov 23, 2007
    #3
  4. Dave Weaver Guest

    On Thu, 22 Nov 2007 15:21:45 -0800 (PST),
    <> 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();
    Dave Weaver, Nov 23, 2007
    #4
  5. On Nov 22, 3:21 pm, "" <> 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
    comp.llang.perl.moderated, Nov 23, 2007
    #5
  6. Guest

    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, "" <> 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
    , Nov 23, 2007
    #6
  7. 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
    comp.llang.perl.moderated, Nov 23, 2007
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Stuart D. Gathman

    Help beautify ugly heuristic code

    Stuart D. Gathman, Dec 8, 2004, in forum: Python
    Replies:
    14
    Views:
    646
    Stuart D. Gathman
    Jul 25, 2006
  2. Replies:
    10
    Views:
    469
    Default User
    Jul 14, 2006
  3. Replies:
    10
    Views:
    585
    Default User
    Jul 14, 2006
  4. Jim Langston

    Ugly C looking code

    Jim Langston, Aug 26, 2006, in forum: C++
    Replies:
    5
    Views:
    400
    Ian Collins
    Aug 26, 2006
  5. Replies:
    8
    Views:
    500
Loading...

Share This Page