Making program readable

ravi45722 has asked for the wisdom of the Perl Monks concerning the following question:

I have some (huge) files. In that i need to count some events which satisfying my condition. For that I am using all these if else conditions. Can I reduce the code by using any class (OOP) concepts. Here I cant use "sub" because in error sections the @network array, @subscriber array, @system errror is too big to pass to a function. It may slow down or even memory outage sometimes. So, Please show a smart way to achieve the same. Hope you can understand my requirement. I am just incrementing a variable if the condition satisfies.

foreach my $Intermediate_file (@Intermediate_files)
{
    open (INTER,"$Intermediate_file") or print "cannot open file\n";
    while (my $intermediate_line = <INTER>)
    {
        chomp $intermediate_line;
        my @intermediate_array = split(/\|/,$intermediate_line);
        if ($intermediate_array[12] eq "GSM" && $intermediate_array[9]
+ eq "MSubmit")
        {
            $intermediate_MO_cnt++;
        }
        if ($intermediate_array[12] eq "GSM" && $intermediate_array[9]
+ eq "MSubmit" && $intermediate_array[7] eq "MsgAccepted")
        {
            $total_MO_success++;
        }
        if ($intermediate_array[12] eq "SMPP" && $intermediate_array[9
+] eq "ESubmit")
        {
            $intermediate_AO_cnt++;
        }
        if ($intermediate_array[12] eq "SMPP" && $intermediate_array[9
+] eq "ESubmit" && $intermediate_array[7] eq "MsgAccepted")
        {
            $total_AO_success++;
        }
        if ($intermediate_array[12] eq "GSM" && $intermediate_array[13
+] eq "SMPP")
        {
            $total_AT++;
        }
#---------------------------------------ERROR SECTION-----------------
+------------------------------------------------------------#
        if ($intermediate_array[12] eq "GSM")
        {
            if (any { /^\Q$intermediate_array[11]\E$/ } @subscriber_er
+ror)
            {
                $MO_userdep_error++;
            }
            elsif (any { /^\Q$intermediate_array[11]\E$/ } @network_er
+ror)
            {
                $MO_network_error++;
            }
            elsif (any { /^\Q$intermediate_array[11]\E$/ } @system_err
+or)
            {
                $MO_system_error++;
            }
        }
        if ($intermediate_array[12] eq "SMPP")
        {
            if($intermediate_array[11]=~ /SMSC/)
            {
                $AO_ESMEdep_error++ if any { /^\Q$intermediate_array[1
+1]\E$/ } @subscriber_error;
            }
            elsif (any { /^\Q$intermediate_array[11]\E$/ } @network_er
+ror)
            {
                $AO_network_error++;
            }
            elsif (any { /^\Q$intermediate_array[11]\E$/ } @system_err
+or)
            {
                $AO_system_error++;
            }
        }
#---------------------------------END OF ERROR SECTION----------------
+-------------------------------------------------------------#
    }
}
print "Completed the Imtermediate section....\n";
#-------------------------------------POSTPAID SECTION----------------
+-------------------------------------------------------------#

foreach my $Postpaid_file (@Postpaid_files)
{
    open (POST ,"$Postpaid_file")     or print "cannot open file\n";
    while(my $cdr_post_line = <POST>)
    {
        chomp $cdr_post_line;
        my @cdr_post_array = split(/\|/,$cdr_post_line);
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[9] eq "Sub
+mit")
        {
            $cdr_post_MO_cnt++ if any { /^\Q$cdr_post_array[11]\E$/ } 
+@MOresp_error;
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[9] eq "Su
+bmit")
        {
            $cdr_post_AO_cnt++ if any { /^\Q$cdr_post_array[11]\E$/ } 
+@MOresp_error;
        }
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[13] eq "GS
+M" && $cdr_post_array[9] eq "Submit")
        {
            $total_MT_P2P++ unless any { /^\Q$cdr_post_array[11]\E$/ }
+ @MOresp_error;
        }
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[13] eq "GS
+M" && $cdr_post_array[9] eq "Submit" && $cdr_post_array[7] eq "Delive
+red")
        {
            $total_MT_P2P_success++;
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[13] eq "G
+SM" && $cdr_post_array[9] eq "Submit")
        {
            $A2P_FDA_count++;
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[13] eq "G
+SM" && $cdr_post_array[9] eq "Submit" && $cdr_post_array[11] eq "SMSC
+_no_error")
        {
            $A2P_FDA_success_count++;
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[13] eq "G
+SM" && $cdr_post_array[9] eq "Submit")
        {
            $total_MT_A2P++;
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[13] eq "G
+SM" && $cdr_post_array[9] eq "Submit" && $cdr_post_array[7] eq "Deliv
+ered")
        {
            $total_MT_A2P_success++;
        }
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[13] eq "SM
+PP" && $cdr_post_array[7] eq "Delivered")
        {
            $total_AT_success++;
        } 
#----------------------------POST PAID ERROR SECTION------------------
+-----------------------------------------------------------#
        if ($cdr_post_array[12] eq "GSM")
        {
            if (any { /^\Q$cdr_post_array[11]\E$/ } @network_error)
            {
                $MO_network_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @subscriber_err
+or)
            {
                $MO_userdep_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @system_error)
            {
                $MO_system_error++;
            }
        }
        if ($cdr_post_array[12] eq "SMPP")
        {
            if($cdr_post_array[11]=~ /SMSC/)
            {
                $AO_ESMEdep_error++ if any { /^\Q$cdr_post_array[11]\E
+$/ } @subscriber_error;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @network_error)
            {
                $AO_network_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @system_error)
            {
                $AO_system_error++;
            }
        }
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[13] eq "GS
+M")
        {
            if (any { /^\Q$cdr_post_array[11]\E$/ } @network_error)
            {
                $MT_P2P_network_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @subscriber_err
+or)
            {
                $MT_P2P_userdep_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @system_error)
            {
                $MT_P2P_system_error++;
            }
        }
        if ($cdr_post_array[12] eq "SMPP" && $cdr_post_array[13] eq "G
+SM")
        {
            if (any { /^\Q$cdr_post_array[11]\E$/ } @network_error)
            {
                $MT_A2P_network_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @subscriber_err
+or)
            {
                $MT_A2P_userdep_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @system_error)
            {
                $MT_A2P_system_error++;
            }
        }
        if ($cdr_post_array[12] eq "GSM" && $cdr_post_array[13] eq "SM
+PP")
        {
            if (any { /^\Q$cdr_post_array[11]\E$/ } @network_error)
            {
                $AT_network_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @subscriber_err
+or)
            {
                $AT_userdep_error++;
            }
            elsif (any { /^\Q$cdr_post_array[11]\E$/ } @system_error)
            {
                $AT_system_error++;
            }
        }
#=====================================Submission failure Post paid sec
+tion===============================================#
        if ($cdr_post_array[11] eq "$db_user_errors[0]")
        {
            $user_first_error++;
        }
        elsif ($cdr_post_array[11] eq "$db_user_errors[1]")
                {
                        $user_second_error++;
                }
        elsif ($cdr_post_array[11] eq "$db_user_errors[2]")
                {
                        $user_third_error++;
                }
        elsif ($cdr_post_array[11] eq "$db_user_errors[3]")
                {
                        $user_forth_error++;
                }
        elsif ($cdr_post_array[11] eq "$db_user_errors[4]")
                {
                        $user_fifth_error++;
                }
        if ($cdr_post_array[11] eq "$db_ESME_errors[0]")
                {
                        $ESME_first_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_ESME_errors[1]")
                {
                        $ESME_second_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_ESME_errors[2]")
                {
                        $ESME_third_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_ESME_errors[3]")
                {
                        $ESME_forth_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_ESME_errors[4]")
                {
                        $ESME_fifth_error++;
                }
           if ($cdr_post_array[11] eq "$db_system_errors[0]")
                {
                        $system_first_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_system_errors[1]")
                {
                        $system_second_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_system_errors[2]")
                {
                        $system_third_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_system_errors[3]")
                {
                        $system_forth_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_system_errors[4]")
                {
                        $system_fifth_error++;
                }    
        if ($cdr_post_array[11] eq "$db_network_errors[0]")
                {
                        $network_first_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_network_errors[1]")
                {
                        $network_second_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_network_errors[2]")
                {
                        $network_third_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_network_errors[3]")
                {
                        $network_forth_error++;
                }
                elsif ($cdr_post_array[11] eq "$db_network_errors[4]")
                {
                        $network_fifth_error++;
                }

#==============================Delivery failure postpaid section======
+=========================================#
        if ($cdr_post_array[11] eq "$del_user_errors[0]")
        {
            $del_user1_error++;
        }
                elsif ($cdr_post_array[11] eq "$del_user_errors[1]")
                {
                        $del_user2_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_user_errors[2]")
                {
                        $del_user3_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_user_errors[3]")
                {
                        $del_user4_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_user_errors[4]")
                {
                        $del_user5_error++;
                }
                if ($cdr_post_array[11] eq "$del_network_errors[0]")
                {
                        $del_network1_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_network_errors[1]"
+)
                {
                        $del_network2_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_network_errors[2]"
+)
                {
                        $del_network3_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_network_errors[3]"
+)
                {
                        $del_network4_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_network_errors[4]"
+)
                {
                        $del_network5_error++;
                }
                if ($cdr_post_array[11] eq "$del_system_errrors[0]")
                {
                        $del_system1_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_system_errrors[1]"
+)
                {
                        $del_system2_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_system_errrors[2]"
+)
                {
                        $del_system3_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_system_errrors[3]"
+)
                {
                        $del_system4_error++;
                }
                elsif ($cdr_post_array[11] eq "$del_system_errrors[4]"
+)
                {
                        $del_system5_error++;
                }
#---------------------------------END OF ERROR SECTION----------------
+-------------------------------------------------------------#

    }
}
print "Completed postpaid section....\n";
[download]

Comment on Making program readable Download Code

Replies are listed 'Best First'.

Re: Making program readable
by Athanasius (Archbishop) on Oct 06, 2015 at 07:42 UTC

Hello ravi45722,

Here I cant use "sub" because in error sections the @network array, @subscriber array, @system errror is too big to pass to a function. It may slow down or even memory outage sometimes.

In Perl, arguments to subroutines are passed by reference, not by value. Within the sub, the special variable @_ contains aliases to the passed-in arguments. It’s usual practice to begin by making local copies:

my ($arg1, $arg2, ...) = @_;
[download]

but that isn’t required: you can always access the arguments directly as $_[0], $_[1], etc.

OK, so there’s still some overhead involved in making the aliases (I haven’t benchmarked it). But you can bypass most of this overhead by passing an array reference instead of an array:

sub some_function
{
    my ($network_ref, $subscriber_ref, $system_ref) = @_;

    for (@$network_ref)
    {
        ...
    }

    ...
}

...

some_function(\@network_error, \@subscriber_error, \@system_error);
[download]

See perlsub.

Hope that helps,

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

[reply]
[d/l]
[select]

Re: Making program readable
by RichardK (Parson) on Oct 06, 2015 at 08:36 UTC

I have no OO techniques to suggest, just write clearer code.

Don't repeat yourself so much. This :-

        if ($intermediate_array[12] eq "GSM" && $intermediate_array[9]
+ eq "MSubmit")
        {
            $intermediate_MO_cnt++;
        }
        if ($intermediate_array[12] eq "GSM" && $intermediate_array[9]
+ eq "MSubmit" && $intermediate_array[7] eq "MsgAccepted")
        {
            $total_MO_success++;
        }
[download]

could be written as :-

    if ($intermediate_array[12] eq "GSM" && $intermediate_array[9] eq 
+"MSubmit") {

        $intermediate_MO_cnt++;
        if ( $intermediate_array[7] eq "MsgAccepted") {
            $total_MO_success++;
        }
    }
[download]

It's clearer and easier to see what your intent was, and there are fewer places for bugs to hide.

Variable names like $del_user1_error , $del_user2_error, .. $del_userN_error are always a sign that you should have used any array and you're writing too much code. So instead you could do something like this :-

    my @del_user_error_count;

    for my $i (0..4) {
        if ($cdr_post_array[11] eq $del_user_errors[$i]) {
            $del_user_error_count[$i]++;
            last;
        }
    }
[download]

[reply]
[d/l]
[select]

Re^2: Making program readable

by ravi45722 (Pilgrim) on Oct 06, 2015 at 10:47 UTC

I replaced the my if-else loops with these.

    my @del_user_error_count;

    foreach my $i (0..4) {
        if ($cdr_post_array[11] eq $del_user_errors[$i]) {
            $del_user_error_count[$i]++;
            last;
        }
    }
[download]

Its decreased 42 lines but its increasing the execution time. I see this through benchmark.

For if-else loop it gives

the code took:52 wallclock secs

After replacing the code with foreach

the code took:67 wallclock secs

[reply]
[d/l]
[select]

Re^3: Making program readable

by karlgoethebier (Abbot) on Oct 09, 2015 at 20:43 UTC

"...I see this through benchmark..."

You might try Iterator::Simple:

#!/usr/bin/env perl

use strict;
use warnings;
# use feature qw (say);
use Iterator::Simple qw(iter);

my $iterator = iter( [ 1 .. 4 ] );

while ( defined( my $i = $iterator->() ) ) {
    # warp action goes here
}
[download]

Regards, Karl

�The Crux of the Biscuit is the Apostrophe�

[reply]
[d/l]

Re^3: Making program readable

by Anonymous Monk on Oct 07, 2015 at 01:43 UTC

Its decreased 42 lines but its increasing the execution time. I see this through benchmark.

Ignore it, its not important :)

[reply]

Re^2: Making program readable

by ravi45722 (Pilgrim) on Oct 06, 2015 at 09:18 UTC

Ya... I understand what you said. Thank you. I will change it. Is there any more changes that can increase my program readability or to reduce the code???

[reply]

Re^3: Making program readable

by Anonymous Monk on Oct 07, 2015 at 01:50 UTC

Is there any more changes that can increase my program readability or to reduce the code???

What does your program look like now ravi45722?

[reply]

Re: Making program readable
by graff (Chancellor) on Oct 07, 2015 at 03:45 UTC

I have no way of testing this proposal in a relevant way, but I think the layout is basically in line with what you're trying to do. (Note that I'm renaming your "$AO_ESMEdep_error" to a hash key spelled "AO_userdep_error", to maintain parallelism between the GSM and SMPP conditions - I hope that doesn't warp things too much for you (the parallelism is worth it, I think).

my %tallies;
my %errors = {
    userdep_error => \@subscriber_error,
    network_error => \@network_error,
    system_error => \@system_error,
};

sub do_GSM
{
    my $fields = shift;
 
    if ( $$fields[9] eq 'MSubmit' ) {
        $tallies{MO_count}++;
        if ( $$fields[7] eq 'MsgAccepted' ) {
            $tallies{MO_success]++;
        }
    }
    if ( $$fields[13] eq 'SMPP' ) {
        $tallies{AT}++;
    }

    for my $type ( keys %errors ) {
        if ( any { /^\Q$$fields[11]\E$/ } @{$errors{$type}} ) {
            $tallies{"MO_$type"}++;
        }
    }
}

sub do_SMPP
{
    my $fields = shift;

    if ( $$fields[9] eq 'ESubmit' ) {
        $$tallies{AO_count}++;
        if ( $$fields[7] eq 'MsgAccepted' ) {
            $tallies{AO_success}++;
        }
    }
    if ( $$fields[11] =~ /SMSC/ ) {
        for my $type ( keys %errors ) {
            if ( any { /^\Q$$fields[11]\E$/ } @{$errors{$type}} ) {
                $tallies{"AO_$type"}++;
            }
        }
    }
}

my %subs = ( GSM => \&do_GSM, SMPP => \&do_SMPP );

foreach my $Intermediate_file (@Intermediate_files)
{
    unless ( open (INTER, $Intermediate_file )) {
        warn "open failed for $Intermediate_file\n";
        next;
    }
    while (<INTER>)
    {
        chomp;
        my @fields = split(/\|/);
        &{$subs{$fields[12]}}( \@fields );
    }
}
print "Completed the Imtermediate section....\n";
...
[download]

[reply]
[d/l]

Re^2: Making program readable

by ravi45722 (Pilgrim) on Oct 07, 2015 at 04:28 UTC

Thank you very much for trying to simplify my code. But I think you aware of GSM & SMPP differences. In GSM (Person to person) there is user_errors. In SMPP (Application to person) there is ESME_errors. We cannot club them. It's totally different. Thanks once again for reply.

[reply]

A reply falls below the community's threshold of quality. You may see it by logging in.


"be consistent"
	PerlMonks