他にもつっこみどころはあるのですが今回は、loop controlに的を絞って。
[H.Iさんのコメントを受け加筆訂正アリ]
まずは、DCONWAY先生の一言から。Loopに関する黄金則です。Perl以外でも有効。
Reject As many iterations as possible, as early as possible. 繰り返しは排除せよ、それもなるべく早い段階で
以下をご覧下さい。
jnaoyaのはてな日記 - 添削その2while (my $log = $logs->readline()) { if ($log->{ua}) { my @jitensha = $log->{ua} =~ $reg; next unless @jitensha; my $pattern; for my $i (0 .. $#jitensha) { if (defined $jitensha[$i]) { $pattern = $analyzeval[$i]; last; } } # Do hoge hoge for $pattern } }
無駄にインデントが深いですね。まず最初のifは、こうしちゃいましょう。
next unless $log->{ua};
DCONWAY流に、unlessを排除したければ、
next if not $log->{ua};
ですが、私はunlessが好きなので上の方ですね。
my @jitensha = $log->{ua} =~ $reg; next unless @jitensha;
そうそう、この調子。ですがここも実はこう出来ます。
my (@jitensha) = ($log->{ua} =~ $reg) or next;
for my $i (0 .. $#jitensha) { if (defined $jitensha[$i]) { $pattern = $analyzeval[$i]; last; } }
とりあえず機械的にLoopを洗うと
for my $i (0 .. $#jitensha) { next unless defined $jitensha[$i]; $pattern = $analyzeval[$i]; last; }
となりますが、実はLoopそのものが不要で、
my @indice = grep { $jitensha[$_] } (0 .. $#jitensha); $pattern = $analyzeval[$indice[0]];
さらに
$pattern = $analyzeval[(grep { $jitensha[$_] } (0 .. $#jitensha))[0]];
と出来ますが、いくらなんでもこれはやりすぎ。ですが、これも標準モジュールのList::Utilのfirst
を使えば、
$pattern = $analyzeval[ first { $jitensha[$_] } (0 .. $#jitensha) ];
と、意味的にもスッキリします。さらに、H.Iさんのcommentどおり
$pattern = $analyzeval[ firstidx { $_ } @jitensha ];
とすればさらにスッキリ。ですがfirstidx
が実装されているList::MoreUtilsは標準装備ではないのですね。CPANから取ってくる必要があります。
標準モジュールで提供されているロジックを使おうということは、DCONWAY先生も
Use the "non-builtin builtins"
「ビルトインでないビルトイン」関数を使え
とおっしゃっています。「要件を満たすリスト中の最初の要素」というのは、まさにList::Util::first()
の仕事で、さらにここで欲しいのは「要件を満たすリスト中の最初の要素のインデックス」なのでList::Util::firstidx()
がドンピシャリです。
ここまでをまとめると以下のようになります。
use List::Util qw/first/; while (my $log = $logs->readline()) { my $ua = $log->{ua} or next; my ($jitensha) = ($ua =~ $reg) or next; $pattern = $analyzeval[ first { $jitensha[$_] } (0 .. $#jitensha) ]; # or # use List::Util qw/firstidx/; # my $pattern = $analyzeval[ firstidx { $_ } @jitensha ]; # Do hoge hoge for $pattern }
ちなみに、
ここギコ!: 安易なループは慎むべきですねwhile (my $log = $logs->readline()) { foreach my $reg (keys %analyze) { if (($log->{ua}) && ($log->{ua} =~ /$reg/)) { my $pattern = $analyze{$reg}; # Do hoge hoge for $pattern goto OUT; } OUT: } }
は論外。goto OUT
はCではとにかくperlでは使ってはいけないタイプのgoto。last
の一言でいいのですから。
Dan the Loop Controller
おっとっと。そうでした。というわけで、記事を更新。
Dan the Man with Too Many Modules to Browse