Рефакторинг существующих Perl Sub

Question

У меня есть 2 sub s в моем Perl-коде, которые почти идентичны.Мне любопытно, как SO реорганизовал бы их, чтобы сделать один sub.

Единственная реальная разница в них - это регулярное выражение и запрос с помощью подготовленного оператора.Подготовленные утверждения также принимают разные параметры.

Мысли?

sub showcaseViewsSubData {
    my ($api_call, $stat_section, $idsite, $prev_date, $last_of_month, $params, $subtable) = @_;

    return unless ($subtable);

    my %sub_params = %{ clone ($params) };
    $sub_params{'idSubtable'} = $subtable->{'idsubdatatable'};

    # $data contains views for each primary showcase page
    my $data = &fetchStatsData($api_call, \%sub_params);

    foreach my $visit_group (@$data) {

        # ignore product pages
        next if ($visit_group->{'url'} && $visit_group->{'url'} =~ /\/products?\//);

        # if ($visit_group->{'url'} && $visit_group->{'url'} =~ /inthenews|pressreleases|downloads/) {
        if ($visit_group->{'idsubdatatable'}) {
            &showcaseViewsSubData($api_call, $stat_section, $idsite, $prev_date, $last_of_month, $params, $visit_group);
            next;
        }

        my $division_name;

        if ($visit_group->{'url'}) {
            my $showcase_id = $visit_group->{'url'};
            $showcase_id =~ s/^.*?\/(\d+)\/.*$/$1/;

            $division_by_showcase_id_sth->execute($showcase_id);
            ($division_name) = $division_by_showcase_id_sth->fetchrow_array();

        } else {
            $visit_group->{'orig_label'} = $visit_group->{'label'};
            $visit_group->{'label'} =~ s/-/%/g;
            $visit_group->{'label'} =~ s|^/||g;
            $visit_group->{'label'} .= '%';
            $division_sth->execute($visit_group->{'label'});
            ($division_name) = $division_sth->fetchrow_array();
        }

        if ($division_name) {

            ## no idea why this is nb_hits, and not nb_actions, like every other method
            my @data_value = ( { 'nb_actions' => ($visit_group->{'nb_hits'} || $visit_group->{'nb_visits'}), 'label' => $division_name } );
            &updateCompanyStats($idsite, 'showcase', $prev_date, \@data_value);
        }
    }
    return 1;
}

и

sub researchViewsSubData {
    my ($api_call, $stat_section, $idsite, $prev_date, $last_of_month, $params, $subtable) = @_;

    return unless ($subtable);

    my %sub_params = %{ clone ($params) };
    $sub_params{'idSubtable'} = $subtable->{'idsubdatatable'};

    # $data contains views for each primary showcase page
    my $data = &fetchStatsData($api_call, \%sub_params);

    if (ref $data ne 'ARRAY') {
        carp "$api_call returned something not an array!";
        return 0;
    }

    foreach my $visit_group (@$data) {
        # if ($visit_group->{'url'} && $visit_group->{'url'} =~ /inthenews|pressreleases|downloads/) {
        if ($visit_group->{'idsubdatatable'}) {
            &researchViewsSubData($api_call, $stat_section, $idsite, $prev_date, $last_of_month, $params, $visit_group);
            next;
        }

        my $division_name;

        if ($visit_group->{'url'}) {
            my $tag_id = $visit_group->{'url'};
            $tag_id =~ s/^.*?\/(\d+)\/.*$/$1/;

            next if ($tag_id !~ /^\d+$/);

            $division_by_tag_id_sth->execute(int($tag_id), int($idsite));
            ($division_name) = $division_by_tag_id_sth->fetchrow_array();

        } else {
            carp Dumper($visit_group);
        }

        if ($division_name) {
            ## no idea why this is nb_hits, and not nb_actions, like every other method
            my @data_value = ( { 'nb_actions' => ($visit_group->{'nb_hits'} || $visit_group->{'nb_visits'}), 'label' => $division_name } );

            &updateCompanyStats($idsite, 'research', $prev_date, \@data_value);
        }
    }
    return 1;
}

Answer 1

Как сказал Синан, вы должны начать с нуля и разработать новый класс или модуль (или их набор) для работы с необходимой вам функциональностью.

Ниже приведено кое-что, что я создал на основе вашего кода. Он предназначен только для того, чтобы дать вам направление.

package MyCompanyStats;

sub process_stats {
    my $params = shift;
    my $data = fetchStatsData($params);
    foreach my $visit_group (@$data) {
        process_stats_group($visit_group);
    }
}

sub process_stats_group {
    my $group = shift;
    if ($group->{'url'}) {
        my $showcase_id = get_showcase_id($group);
        save_by_showcase_id($showcase_id, $group);
    } else {
        my $group_label = get_group_label($group);
        save_by_label($group_label, $group);
    }
}

sub get_group_label {
    my $group = shift;
    my $label = $group->{'label'};
    for ($label) {
        s/-/%/g;
        s|^/||g;
    }
    $label .= '%';
    return $label;
}

sub get_showcase_id {
    my $group = shift;
    my $showcase_id = $visit_group->{'url'};
    $showcase_id =~ s/^.*?\/(\d+)\/.*$/$1/;
    return $showcase_id;
}

1;

Answer 2

Когда вы перевозите около этого большого багажа, это хороший случай, когда вам нужны какие-то предметы.Если не считать этого, вы можете упаковать все это в один хеш "bag" или использовать параметры именованных пар - по крайней мере, так будет выглядеть лучше.

Из кода я не могу сказать, почему вы вызываетекаждый, так что я поместил неуклюжий в хеш с именем theDifference.

use Params::Util qw<_ARRAY _HASH>;

sub viewsSubData {
    my %params = @_ % 2 ? %{ &_HASH } : @_;

    # we delete because 1. we don't pass it, and we use it once.
    return unless my $subtable = delete $params{subtable};

    # If we only want a hashref to pass to fetchStatsData then 
    # stream params and the desired value in a hashref, and we're done.
    # don't need the clone() call because listing out the hash takes care of that.
    # $data contains views for each primary showcase page
    my $data 
        = fetchStatsData( 
          $params{api_call}
        , { %{ $params{params} }
          , idSubtable => $subtable->{'idsubdatatable'}
          }
        );

    # This was made standard--because the loop will fail with the derefence, anyway
    if ( _ARRAY( $data )) {
        # returning undef is for bad states is standard in Perl
        carp( "$api_call returned something not an array!" ) and return;
    }

    my $is_showcase = $params{theDifference};

    foreach my $visit_group (@$data) {

        # ignore product pages
        next if  $is_showcase
             and $visit_group->{'url'}
             and $visit_group->{'url'} =~ /\/products?\//
             ;

        # if ($visit_group->{'url'} && $visit_group->{'url'} =~ /inthenews|pressreleases|downloads/) {
        if ($visit_group->{'idsubdatatable'}) {
            showcaseViewsSubData( %params, subtable => $visit_group);
            next;
        }

        my $division_name;

        if ( $visit_group->{'url'} ) {
            my ( $tag_id ) = $visit_group->{'url'}=~ m{/(\d+)/};

            $division_by_tag_id_sth->execute( $tag_id, ( $is_showcase ? () : int( $params{idsite} ));
            ($division_name) = $division_by_tag_id_sth->fetchrow_array();

        } 
        elsif ( $is_showcase ) {
            # orig_label seems to do nothing
            for ( $visit_group->{label} ) { 
                s|^/||;
                s/-/%/g;
            }
            $division_sth->execute( $visit_group->{'label'} . '%' );
            ($division_name) = $division_sth->fetchrow_array();
        }
        else {
            carp Dumper( $visit_group ) . "\n ";
        }

        if ($division_name) {

            ## no idea why this is nb_hits, and not nb_actions, like every other method
            my @data_value 
                = { nb_actions => ( $visit_group->{'nb_hits'} || $visit_group->{'nb_visits'} )
                  , label      => $division_name 
                  };
            updateCompanyStats( $idsite, @params{ qw<theDifference prev_date> }, \@data_value );
        }
    }
    return 1;
}

И вы бы назвали это так:

viewsSubData(
    { theDifference => $whatever ? 'showcase' : 'research'
    , api_call      => $api_call
    , idsite        => $idsite
    , prev_date     => $prev_date
    , params        => $params
    , subtable      => $subtable
    # neither of these were used.
    #, last_of_month => ??
    #, stat_section  => ??
    });