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:
- Line 2-36:
use
statements, main script plus open_file()
function.
- 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:
main.pl
uses My::CsvAnalyzer
to generate a GUI and produce a report.
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.
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:
- Line 14:
my $testType
- 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:
- Line 16:
my $code_font = $mw->fontCreate('code', -weight => 'bold',-size => 12)
- 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:
- 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.
- 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;
perltidy
would make it easier for folks to follow. \$\endgroup\$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\$