6
\$\begingroup\$

I made a Perl script to analyze CSV files. However, it is running a little slow. Is there anything that I can change to make it more efficient?

#!/usr/bin/perl -w
use strict;
use warnings;
use Time::Piece;
use List::Util qw( min max );
use List::MoreUtils qw(first_index);
use Tk;
use CGI;


my $current_file=();

my $mw = new MainWindow;
my $testType; 

my $code_font = $mw->fontCreate('code', -weight => 'bold',-size => 12);
my $cb;
my $right_frame;
my $title = $mw -> Label(-text=>"\nLog File Analyzer\n", -font => 'code' ) -> pack();
my $label = $mw -> Label(-text=>"To run this generator the columns of your LogFile should be the following:\nIndex - Time - Summary - ID - DataLength - Data...\nIf this is not the format of the file it will not work.\n") -> pack();
my $browseButton = $mw -> Button(-text => "Browse", -command => \&open_file)-> pack();
   $cb = $mw->Checkbutton(-text => 'Generator Test',-variable => \$testType, -onvalue => 1 , -offvalue => 0)->pack();
   $cb = $mw->Checkbutton(-text => 'Other', -onvalue => 2, -offvalue => 0)->pack();
my $generateButton = $mw -> Button(-text => "Generate Report", -command => \&generate_report)->pack;
my $quitButton = $mw -> Button(-text => "Quit",-command => sub { exit })-> pack( -side => 'bottom');

sub open_file
{
   my @types =
       (["CSV files", [qw/.csv /]],
        ["All files",        '*'],
       );
   $current_file= $mw->getOpenFile(-filetypes => \@types);
   print "$current_file\n";
}

sub generate_report
{
    sub str2time 
    {
    my ($str) = @_;
    $str =~ s/(\.[0-9]+)?\z//;
    my $fraction = $1 || 0;
    return Time::Piece->strptime($str, '%H:%M:%S')->epoch + $fraction;
    }


#Variables
my $idNumber;
my $dataLength;
my $timeIntervals;
my $numberOfUniqueIDs;
my $numberOfMessages = 0;
my $initialTime;
my $finalTime;
my $timeLength;
my $firstNumber;
my $secondNumber;
my $differenceIntervals;
my $min;
my $max;
my $lineOfMin1;
my $lineOfMin2;
my $lineOfMax1;
my $lineOfMax2;
my $largestDLCMessage;
my $lineOfMaxDLC;

my %seen;
my @idArray;
my @dataLength;
my @uniqueIDs;
my @timeIntervals;
my @sortedTimeIntervals;
my @differenceIntervals;
my @sortedDataLength;


#Stating the files we are reading/writing
my $file = "$current_file";
my $reportFile = 'LogFile_Report_GUI.txt';

#Opening the file that we are going to read from
open(my $fh, '<', $file) or die "Could not open file $file for reading because: $!";
#Opening the file that we are going to write to
open(my $wh, '>', $reportFile) or die "Could not open $reportFile because: $!";

if(!eof $fh)
{
    my $line = readline $fh;
}

#Header for the report
print $wh "Classic Log File Report\n\n";

while(!eof $fh)
{
    my $line = readline $fh;
    chomp $line;

    #This for loop will increment the counter for every CAN message sent
    foreach ($line)
    {
        $numberOfMessages++;
    }

    #first break line into components
    my @array = split(" ", $line);#space is used as delimiter

    #idNumber
    $idNumber = substr($array[8], " ");

    #dataLenth

    $dataLength = substr($array[11]," ");

    #Adding IDs into an array
    foreach ($line)
    {
        push(@idArray, $idNumber);
    }

    #######################################################

    #Adding DLC from all messages into an array
    foreach ($line)
    {
        push(@dataLength, $dataLength);
    }

    #Getting the largest message in terms of DLC

    $largestDLCMessage = max @dataLength;


    ########################################################

    #Getting the unique ID values
    my %hash = map {$_, 1} @idArray;
    @uniqueIDs = keys %hash;
    $numberOfUniqueIDs = scalar@uniqueIDs;

    #Creating an array with the time intervals
    $timeIntervals = substr($array[4], " ");

    #Adding time intervals to the array
    foreach ($line)
    {
        push(@timeIntervals, $timeIntervals);
    }

    @sortedTimeIntervals = sort @timeIntervals;

}   

$initialTime = $sortedTimeIntervals[0];
$finalTime = $sortedTimeIntervals[-1];

substr($initialTime, 12, 1, '');
substr($finalTime, 12, 1, '');

my $t1 = str2time($initialTime);
my $t2 = str2time($finalTime);

$timeLength = $t2 - $t1;

#Missing a case statement for the format of the time length


    my $loopCounter1 = 0;
    my $loopCounter2 = scalar @timeIntervals - 1;
    my $counter1 = 0;
    my $counter2 = 1;

    #Loop to get the difference in time intervals
    while ($loopCounter1 < $loopCounter2)
    {

    $firstNumber = $timeIntervals[$counter1];
    $secondNumber = $timeIntervals[$counter2];

    substr($firstNumber, 12, 1, '');
    substr($secondNumber, 12, 1, '');

    my $t3 = str2time($firstNumber);
    my $t4 = str2time($secondNumber);   

    $differenceIntervals = abs($t4 - $t3);

        foreach ($differenceIntervals)
        {
            push(@differenceIntervals, $differenceIntervals);
        }       

        $counter1++;
        $counter2++;
        $loopCounter1++;    

        $min = min @differenceIntervals;
        $max = max @differenceIntervals;


        $lineOfMin1 = first_index { $_ eq "$min" } @differenceIntervals;
        $lineOfMin2 = $lineOfMin1 + 1;

        $lineOfMax1 = first_index { $_ eq "$max" } @differenceIntervals;
        $lineOfMax2 = $lineOfMax1 + 1;
        #@sortedDifferenceIntervals = sort @differenceIntervals;

        }

    #Getting the DLC values          
    my %string = map { $_, 1 } @dataLength;


print $wh "\nInitial time: $sortedTimeIntervals[0]\n";
print $wh "\nFinal time(Recorded by the GUI): $sortedTimeIntervals[$#sortedTimeIntervals]\n";
print $wh "\nThe CAN Test took: $timeLength seconds\n";

    if($testType == 1)
    {
        if (grep { $_ eq "0" } @dataLength) 
        {
            print $wh "\nMessages DLC: All messages' data length are zero\n";
        }
        else
        {
            print $wh "\nNot all the messages' data length are zero. This should not be the case when using a generator\n";
        }

    }
    elsif ($testType == 2)
    {   
        if (keys %string == 1)
        {
            print $wh "\nAll messages have the same DLC of $largestDLCMessage\n";
        }
        else
        {
            $lineOfMaxDLC = first_index { $_ eq "$largestDLCMessage" } @dataLength;
            print $wh "\nThe largest message in terms of DLC was: $largestDLCMessage. From CAN DATA Frame $lineOfMaxDLC\n";
            print $wh "\nNumber of ello\n";
        }
    }
    else
    {
        my $label = $mw -> Label(-text=>"\nYou need t select a checkbox!\n") -> pack();
    }



print $wh "\nThe minimun time interval between messages was: $min seconds. Between CAN DATA Frame $lineOfMin1 and $lineOfMin2 \n";
print $wh "\nThe maximun time interval between messages was: $max seconds. Between CAN DATA Frame $lineOfMax1 and $lineOfMax2 \n";
print $wh "\nThe number of unique IDs: $numberOfUniqueIDs\n";
print $wh "\nNumber of messages sent: $numberOfMessages\n";

my $label = $mw -> Label(-text=>"\nDone!\n") -> pack();
}

MainLoop;
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Get rid of unneeded maps, greps and loops in general. You didn't mention how big are your CSV files. \$\endgroup\$
    – mpapec
    Commented Oct 7, 2016 at 19:32
  • 1
    \$\begingroup\$ Could you include a small sample from one of the CSV files? \$\endgroup\$ Commented Oct 8, 2016 at 3:35
  • 2
    \$\begingroup\$ It won't make it more efficient, but running this through perltidy would make it easier for folks to follow. \$\endgroup\$
    – chicks
    Commented Oct 8, 2016 at 11:39
  • \$\begingroup\$ The CSV file are approximately 100k lines long @mpapec \$\endgroup\$ Commented Oct 10, 2016 at 12:02
  • 1
    \$\begingroup\$ What's the purpose of the foreach ($line) loops? As far as I can see, these will not be real loops ( more than one loop iteration) since $line is a scalar. If they only run for a single iteration, why bother with a loop? Or am I missing something here? And please include a small sample of the CSV file in your question. \$\endgroup\$ Commented Oct 11, 2016 at 17:09

1 Answer 1

7
\$\begingroup\$

It is very difficult to read and understand the code, and in addition sample data for the log files that the program is supposed to read and process, is not given. This makes it unnecessarily hard to write a useful review. Anyway, the code illustrates programming habits that in general cannot be recommended, and as such is still a good candidate for a review. I will try to focus on the main points so you can get an idea and improve from there.

As I understand, the program is supposed to read a CSV log file and produce a summary of it. It will give the user some options for the type of summary to produce.

Your main question is:

It is running a little slow. Is there anything that I can change to make it more efficient?

Since the code has so many issues that should be dealt with before any optimization can be considered (elementary programming errors, some logical errors, and also more serious problems in form of bad programming practices) and further realistic input data is missing, I choose not focus on how to improve the speed of the program here. Instead I will focus on programming practices and how to achieve maintainable code.

The current structure of the program can be divided into two parts:

  1. Line 2-36: use statements, main script plus open_file() function.
  2. Line 37-259 : generate_report() function

The first parts sets up the GUI, whereas the second part reads the CSV file and generates the report when the user click the button.

It is quite obvious that the generate_report function has become too long. It has been back-indented as not to disappear off the right end of the screen; it even has an inner sub routine (but not of lexical type!); further all declarations are done at the top of the subroutine, even for variables first used 200 lines below! ..and some of the variables are not used at all. Such a "beast" is obviously very difficult to maintain.

In order to cope with it, a lot of comments have been added to the code. This is a well-known antipattern. One should fix the code by refactoring it, not by commenting it. For a more in-depth treatment, I can recommend the talk Søren Lund at YAPC EU 2016: "Documenting Code Patterns and Anti-Patterns".

So how can the situation be improved? The solution is to provide more structure by refactoring the large function into smaller pieces. Since the generation of the report seems like a well-defined concept and since its implementation requires more than a simple function of, say 60 lines of code, I suggest it is refactored into a separate module, and therein furthered refactored into several subroutines.

The program in itself including the GUI, also seems like an entity that could be reused. I therefore suggest to make that also into a module, leaving us with a refactorization into three source files:

  1. main.pl uses My::CsvAnalyzer to generate a GUI and produce a report.
  2. My/CsvAnalyzer.pm implements the class My::CsvAnalyzer which creates the GUI through a new() and a run() method. It uses an object of class My::CsvAnalyzer::ReportGenerator to generate the report.
  3. My/CsvAnalyzer/ReportGenerator.pm implements the class My::CsvAnalyzer::ReportGenerator which generates the report.

The main difficulty with this refactorization seems to be that the report generator module needs to modify the GUI (which is set up by the analyzer module) and also needs access to the $current_file and $testType variables used by Tk::Button and Tk::Checkbutton. These variables are defined in My/CsvAnalyzer.pm.

The solution I propose here, is to encapsulate the information into an object, i.e. by making My::CsvAnalyzer->new() return an object $analyzer with the two mentioned fields. Now, $analyzer->run() will pass a reference to itself ($self) and the GUI ( $mw ) to My::CsvAnalyzer::ReportGenerator->new(). In this way, the report generator will have access to both the GUI and the variables.

An alternative (and perhaps cleaner solution?) could be to try to make My::CsvAnalyzer::ReportGenerator independent of the GUI, and let the parent My::CsvAnalyzer handle messages and errors that should be displayed in the GUI.

With all that in place (and all other programming errors fixed, see below) you could start doing real improvements to your code like:

  • Improve the design of the GUI
  • How the GUI should report errors (in user input, and in the read CSV file). When using a GUI, error messages should not be written to the parent process's (main.pl) STDERR using die ".." or to its STDOUT using print/say. Instead error messages should be displayed in dialogs or written to error log files.

More specific comments

I will now go through some issues related to style, logical error, and syntax errors in the code. These issues need to be adressed before major improvements can start.

Coding style

Coding style is important to make your code readable, understandable, and maintainable. The guidelines have been developed over decades and teach you about the culture of the language and what is expected of your code if you want to share it with others.

Some guidelines for Perl programming style are described in perlstyle, in Chapter 21 of Programming Perl and in the book Perl Best Practices.

  • Variable-name casing: See wikipedia article on "Snake Case". In Perl, use "snake_case instead" of "camelCase", and be consistent. Example:

    1. Line 14: my $testType
    2. Line 18: my $right_frame


    In the first line, you use "camelCase", whereas in the second you use "snake_case".

  • Don't use the same variable-names for arrays and scalars. Even though it is perfectly legal, and in some cases justifyable, try to avoid using the same variable names for different data types. It makes the code harder to understand. Example: on line 51 you declare the scalar my $timeIntervals; and on line 73 you declare the array my @timeIntervals; similar for line 59 and 75 where you declare $differenceIntervals and @differenceIntervals. Later in the code $timeIntervals is used to index into @timeIntervals. This looks ugly. Instead you could try use the singular form for the scalar, e.g. my $time_interval.

  • Spacing: Spaces between operators are usually recommended for readability, and remember to be consistent with the amount of space you put between operators, see perlstyle for more details. Example:

    1. Line 16: my $code_font = $mw->fontCreate('code', -weight => 'bold',-size => 12)
    2. Line 19: my $title = $mw -> Label(-text=>"\nLog File Analyzer\n", -font => 'code' ) -> pack()
      Here you have put space around some of the -> and => operators, but not all.
  • Braces: Placement of braces. In Perl, "uncuddled elses" and "K&R" style for indenting control constructs is used and recommended. See wikipedia article on indent style and perlstyle. Example of K&R style with uncuddled elses:

    if ( $a == 1 ) {
        ....
    }
    else {
        ....
    }
    

    Example: Line 27-28:

    sub open_file
    {
    

    Rather use this style:

    sub open_file {
    
  • Indentation: Use 4-column indent and be consistent. This helps to make your code more readable. Some places you use 3 space, some place 4 spaces, and some places no indentation at all.

Declarations

  • Position of declarations:
    Lexical variables should be declared as close to their first use as possible. For short subroutines this is usually not an issue (and one should try to keep subroutines short), however for longer subroutines it will help to:

    1. Reduce the amount of code scanning for a maintainer. The maintainer can easily spot the declaration of the variable when he reads the code. He does not need to scroll up hundreds of lines and try to find the place of a variable's declaration and then move down again to where he came from.
    2. Encapsulation. Prevent variables from being accessible (in the same scope) above the point where they are intended to first be used. If you by mistake try to use the variable before it is intended to be used, you will get an error message from the compiler.


    Note: If you need to prevent a variable to be used below a certain point in the code, you can create a scope for it using braces, i.e. : { # lines of code }.

  • Declaration of scalars: To declare a scalar variable $var, you simply write my $var. It is not necessary (and idiomatic) to set it explicitly to undef. However, in some cases it can be argued that my $var = undef will make it more clear that you intend to check if the variable is defined in subsequent code. Although, the same can be achieved by assigning an empty list my $var = (); it is not idiomatic Perl, and hence it can be confusing to others. Example:

    • Line 11: my $current_file=();


    This should be written as my $current_file; or my $current_file = undef; since there is a chance that the user will try to create a report without having chosen a file. Then the program should check if the variable is undefined.

  • Get rid of unused variables and modules. In some development phases and situation, using time on removing unused variables can be considered premature. But in general, unused variables will confuse others that try to read your code. Example from Line 16:

    my $code_font = $mw->fontCreate('code', -weight => 'bold', -size => 12);
    

    Here $code_font is never used and the statement is better written as:

    $mw -> fontCreate('code', -weight => 'bold', -size => 12);
    

    The same is true about unused modules. On line 8, you have use CGI; but the CGI module is never used.

Opening and reading files

  • Remember to close files when you are finished with them. This is a general advice. It is not strictly necessary to do this since lexical filehandles will get automatically closed when they go out of scope. See How severe is it if we do not close a file in Perl? for more details. Example: on line 84-86 you open two files but you never close them.

  • Reading a filehandle line by line The idiomatic way of doing this is in a while loop using the diamond operator. Example: On lines 96-98:

    while(!eof $fh)
    {
        my $line = readline $fh;
    

    This is better written as:

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

    Also see the caveats in the documentation for eof for more information about why it might not be a good idea to use eof in this case.

Incorrect use of functions

  • substr: The syntax is : substr EXPR, OFFSET, LENGTH. Note that OFFSET cannot be a string different from an integer. Example: Line 111:

    $idNumber = substr($array[8], " ");
    

    This will produce warnings like:

    Argument " " isn't numeric in substr at main.pl line 111.
    

    since you use a space for OFFSET. I am not sure what you are trying to do here. I think maybe just capturing the array element ( $idNumber = $array[8] ) ?

  • Difference between grep and all: grep says if at least one element in a list matches, all (in List::Util tells you if all elements in a list match. Example: Line 222:

    if (grep { $_ eq "0" } @dataLength)   {
        print $wh "\nMessages DLC: All messages' data length are zero\n";
    }
    

    This should rather be:

    if (all { $_ eq "0" } @dataLength)   {
        print $wh "\nMessages DLC: All messages' data length are zero\n";
    }
    

Other things

  • Use of warnings flag in file shebang. The use of -w in the main script shebang #!/usr/bin/perl -w is out-of-style and superseded by the more flexible warnings pragma. use warnings gives you a lot more control of which warnings will be triggered, and also lets you promote some warnings to be fatal errors. It also only applies to the lexical scope you are in, while -w will apply even to modules you load that don't expect to have warnings enabled. See Is there a difference between use warnings and -w? for more information.

  • Indirect object syntax when creating objects: Even though Perl supports the "indirect object" type of method invocation syntax, it is in general discouraged according to perlobj. Example: Line 13:

     my $mw = new MainWindow;
    

    This is better written as a regular method call:

    my $mw = MainWindow->new();
    
  • A logical error. On line 141 you are computing the number of unique IDs inside the loop that is creating the IDs. It is not possible to know the number of unique IDs until after the loop has finished. So the statement should be put after the loop instead.

  • Superfluous for loops iterating over a scalar variable: There are many examples of this pattern in the generate_report() function. For example line 102:

    foreach ($line)
    {
        $numberOfMessages++;
    }
    

    is verbose and confusing and better written as

    $numberOfMessages++;
    

Example of improved code:

The code presented below is only a first step towards code improvements. It illustrates the proposed refactorization. It makes no attempt to optimize anything. Hope this can be a good starting point for you. Good luck!

main.pl

use strict;
use warnings;
use My::CsvAnalyzer;

my $analyzer = My::CsvAnalyzer->new( );

$analyzer->run(); #Non blocking call

My/CsvAnalyzer.pm

package My::CsvAnalyzer;

use strict;
use warnings;
use feature qw(say);

use Tk;
use My::CsvAnalyzer::ReportGenerator;

sub new {
    my ( $class ) = @_;
    my $info = { current_file => 'test.csv', test_type => 1, test_type2 => 0 };
    return bless $info, $class;
}

sub run {
    my ( $self ) = @_;

    my $pid = fork;
    die "fork() failed: $!" unless defined $pid;
    return $pid if $pid;

    my $mw = MainWindow->new();
    $mw -> fontCreate('code', -weight => 'bold', -size => 12);
    $mw -> Label(
    -text => "\nLog File Analyzer\n",
    -font => 'code' ) -> pack();
    $mw -> Label(
    -text=> "To run this generator the columns of your "
           . "LogFile should be the following:\n"
           . "Index - Time - Summary - ID - DataLength - Data...\n"
           . "If this is not the format of the file it will not work.\n") -> pack();
    $mw -> Button(-text => "Browse", -command => sub { $self->_open_file( $mw ) })
      -> pack();
    $mw -> Checkbutton(
        -text     => 'Generator Test',
        -variable => \( $self->{test_type} ),
        -onvalue  => 1,
        -offvalue => 0) -> pack();
    $mw -> Checkbutton(
        -text => 'Other',
        -variable => \( $self->{test_type2} ),
        -onvalue => 1,
        -offvalue => 0) -> pack();

    my $report_generator = My::CsvAnalyzer::ReportGenerator->new( $self, $mw );
    $mw -> Button(
        -text    => "Generate Report",
        -command => sub { $report_generator->gen_report() }) -> pack();
    $mw -> Button(
        -text => "Quit",
        -command => sub { exit }) -> pack( -side => 'bottom');

    MainLoop;
}

sub _open_file {
    my ( $self, $mw ) = @_;

    my @types = (
        ["CSV files",  [qw/.csv /]],
        ["All files",          '*'],
       );
   $self->{current_file} = $mw -> getOpenFile(-filetypes => \@types);
}

1;

My/CsvAnalyzer/ReportGenerator.pm

package My::CsvAnalyzer::ReportGenerator;
use strict;
use warnings;

use List::Util qw( all min max );
use List::MoreUtils qw( first_index );
use Time::Piece;
use Tk;

sub new {
    my ( $class, $analyzer, $mw ) = @_;

    return bless {analyzer => $analyzer, mw => $mw}, $class;
}

sub gen_report {
    my ( $self ) = @_;

    my $analyzer = $self->{analyzer};
    my $mw = $self->{mw};

    my ( $sorted_time_intervals, $data_length, $time_length, $min, $max, $line_of_min1,
         $line_of_min2, $line_of_max1, $line_of_max2, $number_of_unique_ids,
         $number_of_messages, $largest_DLC_message )
      = $self->_read_input_file( $analyzer );

    my $report_file_name = 'LogFile_Report_GUI.txt';
    open ( my $wh, '>', $report_file_name )
      or die "Could not open '$report_file_name' for writing: $!";
    print $wh "Classic Log File Report\n\n";
    print $wh "\nInitial time: $sorted_time_intervals->[0]\n";
    print $wh "\nFinal time(Recorded by the GUI): "
      . "$sorted_time_intervals->[$#$sorted_time_intervals]\n";
    print $wh "\nThe CAN Test took: $time_length seconds\n";

    if ($analyzer->{test_type} == 1) {
        if (all { $_ eq "0" } @$data_length) {
            print $wh "\nMessages DLC: All messages' data length are zero\n";
        }
        else {
            print $wh "\nNot all the messages' data length are zero. "
              . "This should not be the case when using a generator\n";
        }
    }
    elsif ($analyzer->{test_type2} == 1) {   
        my %string = map { $_, 1 } @$data_length;
        if (keys %string == 1) {
            print $wh "\nAll messages have the same DLC of $largest_DLC_message\n";
        }
        else {
            my $line_of_max_DLC 
              = first_index { $_ eq "$largest_DLC_message" } @$data_length;
            print $wh "\nThe largest message in terms of DLC was: "
              . "$largest_DLC_message. From CAN DATA Frame $line_of_max_DLC\n";
            print $wh "\nNumber of ello\n";
        }
    }
    else {
        $mw -> Label(-text => "\nYou need to select a checkbox!\n") -> pack();
    }

    print $wh "\nThe minimun time interval between messages was: "
      ."$min seconds. Between CAN DATA Frame $line_of_min1 and $line_of_min2 \n";
    print $wh "\nThe maximun time interval between messages was: "
      . "$max seconds. Between CAN DATA Frame $line_of_max1 and $line_of_max2 \n";
    print $wh "\nThe number of unique IDs: $number_of_unique_ids\n";
    print $wh "\nNumber of messages sent: $number_of_messages\n";

    close $wh;
    $mw -> Label(-text => "\nDone!\n") -> pack();
}

sub _str2time {
    my ( $self, $str) = @_;
    $str =~ s/(\.[0-9]+)?\z//;
    my $fraction = $1 || 0;
    return Time::Piece->strptime($str, '%H:%M:%S')->epoch + $fraction;
}

sub _read_input_file {
    my ( $self, $analyzer ) = @_;

    my $input_file_name = $analyzer->{current_file};
    open ( my $fh, '<', $input_file_name )
      or die "Could not open file '$input_file_name' for reading: $!";
    my $header_line = <$fh>;

    my @id_array;
    my @data_length;
    my @time_intervals;
    my $number_of_messages = 0;
    while (my $line = <$fh>) {
        chomp $line;
        $number_of_messages++;

        my @array = split " ", $line;
        next if !@array;  # skip empty lines
        if ( @array < 12 ) {
            warn_problem( 'Expected at least 12 fields, got ' . (scalar @array)
                          . '. Skipping line.', $input_file_name, $.);
            next;
        }
        my $id_number = $array[8];
        my $data_length = $array[11];
        push @id_array, $id_number;
        push @data_length, $data_length;
        my $time_interval = $array[4];
        push @time_intervals, $time_interval;        
    }
    close $fh;
    my $largest_DLC_message = max @data_length;
    my %hash = map {$_, 1} @id_array;
    my @unique_IDs = keys %hash;
    my $number_of_unique_ids = scalar @unique_IDs;
    my @sorted_time_intervals = sort @time_intervals;

    my $initial_time = $sorted_time_intervals[0];
    my $final_time = $sorted_time_intervals[-1];

    substr $initial_time, 12, 1, ''; # delete character #12
    substr $final_time, 12, 1, ''; # delete character #12

    my $t1 = $self->_str2time( $initial_time );
    my $t2 = $self->_str2time( $final_time );

    my $time_length = $t2 - $t1;

    my ( $min, $max, $line_of_min1, $line_of_min2, $line_of_max1, $line_of_max2 )
      = $self->_get_time_min_max_values( \@time_intervals ); 

    return ( \@sorted_time_intervals, \@data_length, $time_length, $min, $max,
             $line_of_min1, $line_of_min2, $line_of_max1, $line_of_max2,
             $number_of_unique_ids, $number_of_messages, $largest_DLC_message );
}

#FIXME: Display warnings in GUI not at STDERR of parent..
sub warn_problem {
    my ( $err_str, $fn, $line_no ) = @_;

    warn sprintf( "%s:%d : %s\n", $fn, $line_no, $err_str );
}

# FIXME: display error messages in GUI ( not at STDERR )
sub _get_time_min_max_values {
    my ( $self, $time_intervals ) = @_;

    my $loop_counter1 = 0;
    my $loop_counter2 = scalar @$time_intervals - 1;
    my $counter1 = 0;
    my $counter2 = 1;

    my @difference_intervals;

    while ($loop_counter1 < $loop_counter2) {

        my $first_number = $time_intervals->[$counter1];
        my $second_number = $time_intervals->[$counter2];

        substr $first_number, 12, 1, '';
        substr $second_number, 12, 1, '';

        my $t3 = $self->_str2time( $first_number );
        my $t4 = $self->_str2time( $second_number );   

        my $difference_interval = abs( $t4 - $t3 );

        push @difference_intervals, $difference_interval;

        $counter1++;
        $counter2++;
        $loop_counter1++;
    }
    my $min = min @difference_intervals;
    my $max = max @difference_intervals;

    my $line_of_min1 = first_index { $_ eq "$min" } @difference_intervals;
    my $line_of_min2 = $line_of_min1 + 1;

    my $line_of_max1 = first_index { $_ eq "$max" } @difference_intervals;
    my $line_of_max2 = $line_of_max1 + 1;
    return ( $min, $max, $line_of_min1, $line_of_min2, $line_of_max1, $line_of_max2 );
}


1;
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is good. Well done, and thank you for encouraging snake case identifiers: they are much easier to read by people whose first language isn't Englush. A minor point: $numberOfMessages++ is better written as ++$numberOfMessages. A postfixed ++ will increment the variable and return its original value. The extra code to save the original value is almost certainly optimised out if the return value isn't used, but the prefixed version is closer to the intention. It also puts the "verb" first, and so reads better as English. \$\endgroup\$
    – Borodin
    Commented Nov 20, 2016 at 5:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.