in reply to Re^2: Im I forking properly ?
in thread Im I forking properly ?

Hello leostereo,

Why is it so horrible to read??

Because the haphazard indentation makes it almost impossible to tell, at a glance, where each logical block of code ends. Here’s the original version of sub write_register, with a single comment added:

sub write_register {
my ($date,$client_ip,$client_imsi) = @_;

 $output=qx(snmpwalk -v2c -t1 -c $community $client_ip $snmp_fver 2>&1
+);
 chomp($output);
        if( $output eq "Timeout: No Response from $client_ip" ) {
        return; 
        }
        else{
                   my @result=split(/:/,$output);
                   if ($result[3]){
           $fver=$result[3];
                   $fver=~s/ //g;
                   $fver=~s/\n//g;

                   }else{
            exit;
            }
               }    # X

        if($bsid){
                system "sed -i '/$client_imsi/d' $path/leases_list.txt
+";
        system 'echo "'.$date.','.$client_ip.','.$client_imsi.','. $fv
+er.'" >> '.$path.'/leases_list.txt';
        }
        
}

[download]

Now, quickly: which block of code is terminated by the right brace named X?

Here’s a preliminary clean-up of the code, with consistent indentation:

sub write_register
{
    my ($date, $client_ip, $client_imsi) = @_;

    $output = qx(snmpwalk -v2c -t1 -c $community $client_ip $snmp_fver
+ 2>&1);
    chomp $output;

    if ($output eq "Timeout: No Response from $client_ip")
    {
        return; 
    }
    else
    {
        my @result = split /:/, $output;

        if ($result[3])
        {
            $fver =  $result[3];
            $fver =~ s/ //g;
            $fver =~ s/\n//g;
        }
        else
        {
            exit;
        }
    }   # X

    if ($bsid)
    {
        system "sed -i '/$client_imsi/d' $path/leases_list.txt";
        system 'echo "' . $date . ',' . $client_ip . ',' . $client_ims
+i . ',' .
                          $fver . '" >> ' . $path . '/leases_list.txt'
+;
    }
}

[download]

Easier to follow now? Yes, and that makes it easier to spot where it can be improved:

  1. As noted by Anonymous Monk++, the variables $output and $fver are never declared. In addition, the variable $bsid is neither declared nor initialized anywhere in the script.
  2. Perl allows you to return, exit, or die at any point in the code. Since these statements are forms of flow control, the code can often be simplified when these statements are used. For example, this: 
    if (condition)
{
    return;
}
else
{
    ...
}

    [download]
    is logically equivalent to the shorter and simpler: 
    return if condition;
...

    [download]
  3. It’s bad practice to exit from a subroutine. You should die (i.e., throw an exception) instead: that way, the subroutine’s clients have the option to either handle the exception locally or allow it to propagate upwards and so terminate the script.

Here’s an improved version of the subroutine:

sub write_register
{
    my ($date, $client_ip, $client_imsi, $bsid) = @_;
    chomp(my $output = qx(snmpwalk -v2c -t1 -c $community $client_ip $
+snmp_fver 2>&1));
    return if $output eq "Timeout: No Response from $client_ip";
    my @result = split /:/, $output;
    die unless $result[3];
    my $fver = $result[3];
       $fver =~ s/[ \n]//g;

    if ($bsid)
    {
        system "sed -i '/$client_imsi/d' $path/leases_list.txt";
        system 'echo "' . $date . ',' . $client_ip . ',' . $client_ims
+i . ',' .
                          $fver . '" >> ' . $path . '/leases_list.txt'
+;
    }
}

[download]

As a rule, the shorter and simpler the code, the easier it is to debug and maintain. Disclaimer: I haven’t looked at the code’s logic, only its form.

Hope that helps,

Athanasius <°(((>< contra mundum Iustus alius egestas vitae, eros Piratica,

In Section Seekers of Perl Wisdom