Refacturation des sous-marins existants de perl

Question

J'ai dans mon code perl 2 sub qui sont tous identiques. Je suis curieux de savoir comment SO les refactoriser pour en faire un sub.

La seule vraie différence entre eux est une regex, et une requête via l'instruction préparée. Les déclarations préparées prennent également différents paramètres.

Pensées?

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

et

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

Comme ce qu'a dit Sinan, vous devriez commencer à partir de zéro et de concevoir une nouvelle classe ou d'un module (ou un ensemble d'entre eux) pour faire face à la fonctionnalité dont vous avez besoin.

Ci-dessous quelque chose que j'ai produit en fonction de votre code. Il est juste conçu comme un aperçu pour vous donner une direction.

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

Lorsque vous portez à propos de ce de bagages, il est un bon cas que vous avez besoin des objets. Bref, vous pouvez tout emballer dans un seul hachage "sac" ou utiliser des paramètres de paire nommée - au moins, ça ira mieux de cette façon.

Du code je ne peux pas dire pourquoi vous appelleriez chacun, donc j'ai mis un maladroit dans le hachage appelé 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; 
} 

Et vous appelleriez comme ceci:

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 => ?? 
    });