Как я могу написать Perl, который не похож на C? - PullRequest
5 голосов
/ 11 апреля 2009

Мои коллеги жалуются, что мой Perl слишком похож на C, что естественно, так как я программирую на C большую часть времени, а Perl - немного. Вот мои последние усилия. Меня интересует Perl, который легко понять. Я немного критик Perl и мало толерантен к загадочному Perl. Но с учетом читабельности, как следующий код может быть более Perlish?

Его цель - провести анализ трафика и определить, какие IP-адреса находятся в пределах диапазонов, указанных в файле "ips". Вот мои усилия:

#!/usr/bin/perl -w

# Process the files named in the arguments, which will contain lists of IP addresses, and see if 
# any of them are in the ranges spelled out in the local file "ip", which has contents of the
# form start-dotted-quad-ip-address,end-dotted-quad-ip_address,stuff_to_be_ignored
use English;


open(IPS,"ips") or die "Can't open 'ips' $OS_ERROR";

# Increment a dotted-quad ip address
# Ignore the fact that part1 could get erroneously large.
sub increment {
    $ip = shift;

    my ($part_1, $part_2, $part_3, $part_4) = split (/\./, $ip);
    $part_4++;
    if ( $part_4 > 255 ) {
        $part_4 = 0;
        ($part_3++);
        if ( $part_3 > 255 ) {
            $part_3 = 0;
            ($part_2++);
            if ( $part_2 > 255 ) {
                $part_2 = 0;
                ($part_1++);
            }
        }
   }   
    return ("$part_1.$part_2.$part_3.$part_4");
}

# Compare two dotted-quad ip addresses.
sub is_less_than {
    $left = shift;
    $right = shift;

    my ($left_part_1, $left_part_2, $left_part_3, $left_part_4)     = split (/\./, $left);
    my ($right_part_1, $right_part_2, $right_part_3, $right_part_4) = split (/\./, $right);


    if  ($left_part_1 != $right_part_1 ) { 
        return ($left_part_1 < $right_part_1);
    }   
    if  ($left_part_2 != $right_part_2 ) { 
        return ($left_part_2 < $right_part_2);
    }   
    if  ($left_part_3 != $right_part_3 ) { 
        return ($left_part_3 < $right_part_3);
    }
    if  ($left_part_4 != $right_part_4 ) {
        return ($left_part_4 < $right_part_4);
    }
    return (false);  # They're equal
}

my %addresses;
# Parse all the ip addresses and record them in a hash.   
while (<IPS>) {
    my ($ip, $end_ip, $junk) = split /,/;
    while (is_less_than($ip, $end_ip) ) {
        $addresses{$ip}=1;
        $ip = increment($ip);
    }
}

# print IP addresses in any of the found ranges

foreach (@ARGV) {
    open(TRAFFIC, $_) or die "Can't open $_ $OS_ERROR";
    while (<TRAFFIC> ) {
        chomp;
        if (defined $addresses{$_}) {
            print "$_\n";
        }
    }
    close (TRAFFIC);

}

Ответы [ 15 ]

24 голосов
/ 11 апреля 2009

За годы наблюдения за Perl-кодом, написанным программистами на C, вот несколько общих советов:

Используйте хэши. Используйте списки. ИСПОЛЬЗУЙТЕ HASHES! ИСПОЛЬЗУЙТЕ СПИСКИ! Используйте операции со списками (map, grep, split, join), особенно для небольших циклов. Не используйте модные списочные алгоритмы; pop, splice, push, shift и unshift дешевле. Не используйте деревья; хэши дешевле. Хэши дешевы, делай их, используй их и выбрасывай! Используйте итератор для цикла, а не 3-аргументный. Не называйте вещи $ var1, $ var2, $ var3; используйте вместо этого список. Не называйте вещи $ var_foo, $ var_bar, $ var_baz; используйте вместо этого хэш. Используйте $foo ||= "default". Не используйте $_, если вам нужно набрать его.

Не используйте прототипы, ЭТО ЛОВУШКА !!

Используйте регулярные выражения, а не substr() или index(). Люблю регулярные выражения. Используйте модификатор /x, чтобы сделать их читабельными.

Напишите statement if $foo, если вы хотите безусловное условие. Почти всегда есть лучший способ написать вложенное условие: попробуйте рекурсию, попробуйте цикл, попробуйте хеш.

Объявляйте переменные, когда они вам нужны, а не в верхней части подпрограммы. использовать строгое. используйте предупреждения и исправьте их все. использовать диагностику. Пишите тесты. Написать POD.

Использовать CPAN. Используйте CPAN! ИСПОЛЬЗУЙТЕ CPAN! Кто-то, вероятно, уже сделал это, лучше.

Прогон перкритика . Запустите его с --brutal только для ударов. Запустите perltidy . Подумай, почему ты все делаешь. Измени свой стиль.

Используйте время, не затрачиваемое на борьбу с языком и отладку выделения памяти, чтобы улучшить свой код.

Задавайте вопросы. Примите стильный комментарий к вашему коду. Перейти на встречу Perl Mongers. Перейти на perlmonks.org. Перейти к YAPC или Perl Workshop. Ваши знания Perl будут стремительно расти.

20 голосов
/ 11 апреля 2009

Большая часть написания кода для «Perlish» использует преимущества встроенных в Perl функций.

Например, это:

my ($part_1, $part_2, $part_3, $part_4) = split (/\./, $ip);
$part_4++;
if ( $part_4 > 255 ) {
    $part_4 = 0;
    ($part_3++);
    if ( $part_3 > 255 ) {
        $part_3 = 0;
        ($part_2++);
        if ( $part_2 > 255 ) {
            $part_2 = 0;
            ($part_1++);
        }
    }
}   

Я бы переписал что-то вроде:

my @parts = split (/\./, $ip);

foreach my $part(reverse @parts){
  $part++;
  last unless ($part > 255 && !($part = 0));
}

Это делает то, что делает ваш код, опубликованный выше, но немного чище.

Вы уверены, что код делает то, что вы хотите? Просто мне кажется немного странным, что вы переходите к предыдущей «части» IP, только если после нее> 255.

15 голосов
/ 11 апреля 2009

Иногда наиболее Perlish, что нужно сделать, это обратиться к CPAN вместо того, чтобы писать какой-либо код вообще.

Вот быстрый и грязный пример использования Net :: CIDR :: Lite и Net :: IP :: Match :: Regexp :

#!/path/to/perl

use strict;
use warnings;

use English;
use IO::File;
use Net::CIDR::Lite;
use Net::IP::Match::Regexp qw(create_iprange_regexp match_ip);


my $cidr = Net::CIDR::Lite->new();

my $ips_fh = IO::File->new();

$ips_fh->open("ips") or die "Can't open 'ips': $OS_ERROR";

while (my $line = <$ips_fh>) {

    chomp $line;

    my ($start, $end) = split /,/, $line;

    my $range = join('-', $start, $end);

    $cidr->add_range($range);

}

$ips_fh->close();

my $regexp = create_iprange_regexp($cidr->list());

foreach my $traffic_fn (@ARGV) {

    my $traffic_fh = IO::File->new();

    $traffic_fh->open($traffic_fn) or die "Can't open '$traffic_fh': $OS_ERROR";

    while (my $ip_address = <$traffic_fh>) {

        chomp $ip_address;

        if (match_ip($ip_address, $regexp)) {
            print $ip_address, "\n";
        }     

    }

    $traffic_fh->close();

}

ОТКАЗ ОТ ОТВЕТСТВЕННОСТИ: Я только что это проклял, у него было минимальное тестирование и никакого бенчмаркинга. Проверки работоспособности, обработка ошибок и комментарии опущены, чтобы уменьшить количество строк. Я не сжимал пустые места, хотя.

Что касается вашего кода: вам не нужно определять ваши функции перед их использованием.

14 голосов
/ 11 апреля 2009

Другой пример переписать:

sub is_less_than {
    my $left = shift; # I'm sure you just "forgot" to put the my() here...
    my $right = shift;

    my ($left_part_1, $left_part_2, $left_part_3, $left_part_4)     = split (/\./, $left);
    my ($right_part_1, $right_part_2, $right_part_3, $right_part_4) = split (/\./, $right);


    if  ($left_part_1 != $right_part_1 ) { 
        return ($left_part_1 < $right_part_1);
    }   
    if  ($left_part_2 != $right_part_2 ) { 
        return ($left_part_2 < $right_part_2);
    }   
    if  ($left_part_3 != $right_part_3 ) { 
        return ($left_part_3 < $right_part_3);
    }
    if  ($left_part_4 != $right_part_4 ) {
        return ($left_part_4 < $right_part_4);
    }
    return (false);  # They're equal
}

К этому:

sub is_less_than {
    my @left = split(/\./, shift);
    my @right = split(/\./, shift);

    # one way to do it...
    for(0 .. 3) {
        if($left[$_] != $right[$_]) {
            return $left[$_] < $right[$_];
        }
    }

    # another way to do it - let's avoid so much indentation...
    for(0 .. 3) {
        return $left[$_] < $right[$_] if $left[$_] != $right[$_];
    }

    # yet another way to do it - classic Perl unreadable one-liner...
    $left[$_] == $right[$_] or return $left[$_] < $right[$_] for 0 .. 3;

    # just a note - that last one uses the short-circuit logic to condense
    # the if() statement to one line, so the for() can be added on the end.
    # Perl doesn't allow things like do_this() if(cond) for(0 .. 3); You
    # can only postfix one conditional. This is a workaround. Always use
    # 'and' or 'or' in these spots, because they have the lowest precedence.

    return 0 == 1; # false is not a keyword, or a boolean value.
    # though honestly, it wouldn't hurt to just return 0 or "" or undef()
}

Также здесь:

my ($ip, $end_ip, $junk) = split /,/;

$junk может потребоваться @junk для захвата всех мусора, или , вы, вероятно, можете его отключить - если вы назначите массив неизвестного размера для " массив "из двух элементов, он будет молча отбрасывать все лишние вещи. Так

my($ip, $end_ip) = split /,/;

А здесь:

foreach (@ARGV) {
    open(TRAFFIC, $_) or die "Can't open $_ $OS_ERROR";
    while (<TRAFFIC> ) {
        chomp;
        if (defined $addresses{$_}) {
            print "$_\n";
        }
    }
    close (TRAFFIC);
}

Вместо TRAFFIC используйте переменную для хранения дескриптора файла. Кроме того, в общем, вы должны использовать exists(), чтобы проверить, существует ли элемент хеша, а не defined() - он может существовать, но быть установлен на undef (это не должно происходить в вашей программе, но это хорошая привычка когда ваша программа усложняется):

foreach (@ARGV) {
    open(my $traffic, $_) or die "Can't open $_ $OS_ERROR";
    while (<$traffic> ) {
        chomp;
        print "$_\n" if exists $addresses{$_};
    }
    # $traffic goes out of scope, and implicitly closes
}

Конечно, вы также можете использовать замечательный оператор Perl <>, который открывает каждый элемент @ARGV для чтения и действует как файловый дескриптор, который проходит через них:

while(<>) {
    chomp;
    print "$_\n" if exists $addresses{$_};
}

Как было отмечено ранее, старайтесь избегать use ing English, если вы не use English qw( -no_match_vars );, чтобы избежать значительного штрафа за производительность тех злых match_vars, находящихся там. И, как еще не было отмечено, но должно быть ...

ВСЕГДА ВСЕГДА ВСЕГДА всегда use strict; и use warnings;, иначе Ларри Уолл спустится с небес и нарушит ваш код. Я вижу, что у вас есть -w - этого достаточно, потому что даже вне Unix Perl анализирует строку shebang и найдет ваш -w и будет use warnings;, как и должно быть. Тем не менее, вам нужно до use strict;. Это вызовет много серьезных ошибок в вашем коде, например, не объявляя переменные с my или используя false в качестве ключевого слова языка.

Если ваш код работает под strict, а также с warnings, это приведет к НАМНОГО чистому коду, который никогда не сломается по причинам, которые вы не можете понять. Вы потратите часы на отладку отладчика и, возможно, в конечном итоге будете использовать strict и warnings, чтобы просто выяснить, в чем заключаются ошибки. Только удаляйте их, если (и только если) ваш код закончен и вы выпускаете его, и он никогда не генерирует никаких ошибок.

13 голосов
/ 11 апреля 2009

Хотя это, безусловно, один из способов сделать это в Perl.

use strict;
use warnings;

my $new_ip;
{
  my @parts = split ('\.', $ip);

  foreach my $part(reverse @parts){
    $part++;

    if( $part > 255 ){
      $part = 0;
      next;
    }else{
      last;
    }
  }
  $new_ip = join '.', reverse @parts;
}

Вот как бы я это реализовал.

use NetAddr::IP;

my $new_ip = ''.(NetAddr::IP->new($ip,0) + 1) or die;
6 голосов
/ 11 апреля 2009

Я не могу сказать, что это решение сделает вашу программу более перламутровой, но это может упростить ваш алгоритм.

Вместо того, чтобы рассматривать IP-адрес как число с точками в квадрате, число 256, которому требуется структура nested-if для реализации функции приращения, считайте IP-адрес 32-разрядным целым числом. Преобразуйте IP-адрес вида a.b.c.d в целое число с помощью этого (не проверено):

sub ip2int {
    my $ip = shift;
    if ($ip =~ /(\d+)\.(\d+)\.(\d+)\.(\d+)/) {
        return ($1 << 24) + ($2 << 16) + ($3 << 8) + $4;
    } else {
        return undef;
    }
}

Теперь легко определить, находится ли IP между двумя IP-адресами конечной точки. Просто выполните простую целочисленную арифметику и сравнения.

$begin = "192.168.5.0";
$end = "192.168.10.255";
$target = "192.168.6.2";
if (ip2int($target) >= ip2int($begin) && ip2int($target) <= ip2int($end)) {
    print "$target is between $begin and $end\n";
} else {
    print "$target is not in range\n";
}
5 голосов
/ 11 апреля 2009

Скажите своим коллегам, что их Perl слишком похож на шум линии. Пожалуйста, не запутывайте ваш код только ради запутывания - это такие же цели разработки, как и то, которые дают Perl такую ​​плохую репутацию нечитабельности, когда действительно плохие программисты (очевидно, ваши коллеги) пишут неаккуратный код. Хорошо структурированный, с отступом и логический код - это хорошо. С - хорошая вещь.

Серьезно, хотя - лучшее место, чтобы выяснить, как писать Perl, - это O'Reilly "Perl Best Practices" Дамиана Конвея. Он говорит вам, как он думает, что вы должны что-то делать, и он всегда дает веские основания для своей позиции, а также иногда дает веские основания не соглашаться. Я не согласен с ним по некоторым вопросам, но его аргументация обоснована. Вероятность того, что вы работаете с кем-либо, кто знает Perl лучше, чем мистер Конвей, довольно мала, и наличие печатной книги (или, по крайней мере, подписки на Safari) дает вам более надежную поддержку для ваших аргументов. Возьмите копию книги Perl Cookbook, пока смотрите ее, так как рассмотрение примеров кода для решения типичных проблем должно помочь вам в правильном направлении. Я не хочу говорить "купи книгу", но это исключительно хорошие книги, которые любой разработчик Perl должен прочитать.

Что касается вашего конкретного кода, вы используете foreach, $_, расщепление без паренов, сдвигов и т. Д. Это выглядит довольно перламутрово на мой взгляд - которые довольно долго разрабатывались с помощью perl. Одно замечание - я ненавижу английский модуль. Если вы должны использовать это, сделайте это как use English qw( -no_match_vars );. Опция match_vars значительно замедляет синтаксический анализ регулярных выражений, а предоставляемые им переменные $PREMATCH / $POSTMATCH обычно бесполезны.

4 голосов
/ 11 апреля 2009

Есть только 1 совет: используйте строгое. Остальное вряд ли актуально.

3 голосов
/ 11 апреля 2009

Я точно знаю, что ты чувствуешь. Моим первым языком был ФОРТРАН, и, как хороший программист на ФОРТРАНЕ, я писал на ФОРТРАНЕ на каждом языке, начиная с:).

У меня есть действительно замечательная книга Эффективное программирование на Perl , которую я постоянно перечитываю время от времени. Особенно глава под названием «Идиоматический Perl». Вот несколько вещей, которые я использую для того, чтобы мой Perl выглядел как Perl: операторы списков, такие как map и grep, слайсы и хеш-фрагменты, операторы кавычек.

Еще одна вещь, которая мешает моему Perl выглядеть как FORTRAN / C, это регулярное чтение исходных кодов модулей, особенно тех, что написаны мастерами.

2 голосов
/ 14 апреля 2009

Пока это будет работать:

use strict;
use warnings;
use 5.010;

use NetAddr::IP;

my %addresses;
# Parse all the ip addresses and record them in a hash.
{
  open( my $ips_file, '<', 'ips') or die;

  local $_; # or my $_ on Perl 5.10 or later
  while( my $line = <$ips_file> ){
    my ($ip, $end_ip) = split ',', $line;
    next unless $ip and $end_ip;

    $ip     = NetAddr::IP->new( $ip, 0 ) or die;
    $end_ip = NetAddr::IP->new( $end_ip ) or die;
    while( $ip <= $end_ip ){
      $addresses{$ip->addr} = 1;
      $ip++;
    }
  }
  close $ips_file
}

# print IP addresses in any of the found ranges
use English;

for my $arg (@ARGV) {
  open(my $traffic, '<',$arg) or die "Can't open $arg $OS_ERROR";
  while( my $ip = <$traffic> ){
    chomp $ip;
    if( $addresses{$ip} ){
      say $ip
    }
  }
  close ($traffic);
}

Я бы по возможности использовал сетевые маски, потому что это становится еще проще:

use Modern::Perl;
use NetAddr::IP;

my @addresses;
{
  open( my $file, '<', 'ips') or die;

  while( (my $ip = <$file>) =~ s(,.*){} ){
    next unless $ip;
    $ip = NetAddr::IP->new( $ip ) or die;
    push @addresses, $ip
  }

  close $file
}


for my $filename (@ARGV) {
  open( my $traffic, '<', $filename )
    or die "Can't open $filename";

  while( my $ip = <$traffic> ) {
    chomp $ip;
    next unless $ip;

    $ip = NetAddr::IP->new($ip) or next; # skip line on error
    my @match;
    for my $cmp ( @addresses ){
      if( $ip->within($cmp) ){
        push @match, $cmp;
        #last;
      }
    }

    say "$ip => @match" if @match;

    say "# no match for $ip" unless @match;
  }
  close ($traffic);
}

Тест ips файл:

192.168.0.1/24
192.168.0.0
0:0:0:0:0:0:C0A8:0/128

Тест traffic файл:

192.168.1.0
192.168.0.0
192.168.0.5

Выход:

# no match for 192.168.1.0/32
192.168.0.0/32 => 192.168.0.1/24 192.168.0.0/32 0:0:0:0:0:0:C0A8:0/128
192.168.0.5/32 => 192.168.0.1/24
Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...