Я собираюсь просмотреть этот код, как если бы он был отправлен на Проверка кода .
Сначала вы пишете в Perl, как если бы это было C . Что в целом не так уж и плохо, но это означает, что вы выполняете немного больше работы, чем необходимо.
Вместо использования этого слишком многословного и потенциально интенсивного использования памяти:
my @totalReport
...
while(<$input>) { chomp; push @totalReport, $_; }
foreach(@totalReport)
{
$eachAddr = $_;
...
}
while( my $addr = <$input> ){
chomp $addr;
...
}
Обратите внимание, как я удалил переменную и сделал так, чтобы она зацикливалась на входе один раз, а не дважды. Он также не сохраняет значения в памяти по всей длине программы.
Вместо открытия файла для записи, закрытия и повторного открытия:
my $tempFile;
open $tempFile, '>', $tempFilePath or die "Cannot open 1: $!\n";
print $tempFile get($eachAddr);
close $tempFile;
open $tempFile, '<', $tempFilePath or die "Cannot open 2: $!\n";
open my $tempFile, '+>', $tempFilePath or die "Can't open '$tempFilePath' with mode'+>': '$!'";
print $tempFile get($eachAddr);
seek $tempFile, 0, 0;
Вместо того, чтобы дважды получать текст по указанному URL и использовать странный определенный тест:
$undefCheck = get($eachAddr);
if(defined($undefCheck) == 0) { next; }
...
print $tempFile get($eachAddr);
my $text = get( $addr );
next unless defined $text;
...
print $tempFile $text;
Вместо связки:
open ... or die ...;
Я бы использовал autodie .
use autodie;
...
# will now die on error and will tell you the file it fails on.
open my $fh, '<', $filename;
Еще одна вещь, на которую я хотел бы обратить внимание: die "...\n" не позволяет die добавить местоположение ошибки. Единственный раз, когда вы должны это сделать, это если поведение по умолчанию бесполезно.
Если вы закрыли $tempFile перед проверкой $dup, это может быть проще:
if($dup == 1) { close $tempFile; next; }
close $tempFile;
close $tempFile;
next if $dup;
Вместо этого повторяющегося блока кода:
while(<$iReport>)
{
chomp;
if ($_ =~ /.*\<title>(.*)\<\/title>/) { if($1 ne 'ASF JIRA') { say $oReport "Title : $1"; } }
elsif($_ =~ /.*\<assignee username="(.*)">.*\<\/assignee>/) { say $oReport "Assignee: $1"; }
elsif($_ =~ /.*\<reporter username="(.*)">.*\<\/reporter>/) { say $oReport "Reporter: $1"; }
elsif($_ =~ /.*\<type[^>]*>(.*)\<\/type>/) { say $oReport "Type : $1"; }
elsif($_ =~ /.*\<priority[^>]*>(.*)\<\/priority>/) { say $oReport "Priority: $1"; }
elsif($_ =~ /.*\<created>(.*)\<\/created>/) { say $oReport "Created : $1"; }
elsif($_ =~ /.*\<resolved>(.*)\<\/resolved>/) { say $oReport "Resolved: $1"; }
}
use List::Util qw'max';
my @simple_tags = qw'title type priority created resolved';
my $simple_tags_length = max map length, @simple_tags, qw'assignee reporter';
my $simple_tags = join '|', @simple_tags;
...
while( <$iReport> ){
my($tag,$contents);
if( ($tag,$contents) = /<($simple_tags)[^>]*>(.*?)<\/\g{1}>/ ){
}elsif( ($tag,$contents) = /<(assignee|reporter) username="(.*?)">.*?<\/\g{1}>/ ){
}else{ next }
printf $oReport "%-${simple_tags_length}s: %s\n", ucfirst($tag), $contents;
}
Хотя этот код не короче и не понятнее, было бы очень легко добавить еще один тег для сравнения. Так что на самом деле это не лучше , а менее повторяющееся.
Я хотел бы отметить, что $_ =~ /.../ лучше записать как /.../.
Вы можете использовать or вместо if / elsif / else с пустыми блоками.
...
while( <$iReport> ){
/<($simple_tags)[^>]*>(.*?)<\/\g{1}>/
or /<(assignee|reporter) username="(.*?)">.*?<\/\g{1}>/
or next;
my($tag,$contents) = ($1,$2);
printf $oReport "%-${simple_tags_length}s: %s\n", ucfirst($tag), $contents;
}
Лучше всего объединить их в одно регулярное выражение, используя синтаксис /x и (?<NAME>REGEX) с %- или %+.
...
while( <$iReport> ){
/
(?:
# simple tags
< (?<tag> $simple_tags ) [^>]* >
# contents between tags
(?<contents> .*? )
|
# tags with contents in `username` attribute
<
(?<tag> assignee|reporter )
[ ]
# contents in `username` attribute
username = "(?<contents> .*? )"
>
.*? # throw out stuff between tags
)
<\/ \g{tag} > # end tag matches start tag
/x or next; # skip if it doesn't match
printf $oReport "%-${simple_tags_length}s: %s\n", ucfirst($+{tag}), $+{contents};
}
Или даже используйте (DEFINE) (я оставлю это как упражнение для читателя, так как это уже довольно долго).
Возможно, худшая часть кода в том, что вы определяете почти все переменные заранее.
my @totalReport;
my $eachAddr;
my $copyFile;
my $copyFilePath = 'c:\perl\015_JiraGet\HADOOP XML\\';
my $tempFile;
my $tempFilePath = 'c:\perl\015_JiraGet\temp.txt';
my $analyzed;
my $analyzedPath = 'c:\perl\015_JiraGet\analyzed - HADOOP.txt';
my $undefCheck;
my $i = 0;
my $j = 0;
my $title = 'temp';
my $dup = 0;
Это означает, что вы практически используете глобальные переменные. Хотя некоторые из них, по-видимому, должны быть определены там, некоторые из них этого не делают, и поэтому их не следует определять там. Вы действительно должны определять свои переменные в той точке, в которой они вам нужны, или, по крайней мере, в начале блока, где они вам нужны.
Причина, по которой вы не получаете вывод до тех пор, пока файл не будет закрыт, заключается в том, что Perl буферизирует вывод.
Обычно Perl буферизует вывод, поэтому он не выполняет системный вызов для каждого бита вывода. Сохраняя вывод, он делает меньше дорогих системных вызовов. ...
- perlfaq5
Старый способ отключить буферизацию - выбрать файл для вывода и установить $| в ненулевое значение, а затем повторно выбрать исходный вывод.
{
my $previous_default = select($file); # save previous default output handle
$| = 1; # autoflush
select($previous_default); # restore previous default output handle
}
Новый способ заключается в использовании $file->autoflush, который исходит от IO :: Handle .
(Модуль будет автоматически загружен для вас в последних версиях Perl 5)
Вы также можете очистить выход при выборе, используя flush или $file->flush.
IO :: Handle также добавляет $file->printflush, который временно включает автоматическую очистку во время печати.