HiveBrain v1.2.0
Get Started
← Back to all entries
snippethtmlMinor

Readability of this Perl script to generate HTML code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisscriptperlreadabilitygeneratecodehtml

Problem

I'm writing some scripts for a project which need to be maintainable. Someone will be coming in and taking over after a while and I want it to be easy for them to understand what's going on.

What it does is take some arguments with details about a pie chart. It opens and processes a CSV file to be formatted for a javascript Highchart. The user inputs a position for the chart, and css borders and positioning are created based on that, etc... The final product is an HTML file containing the newly created chart.

My question is, would you be able to read this and understand what it does without me explaining it at all?

```
#!/usr/bin/perl

use strict;
use warnings;

require 'makeThousands.pl';

# sub genHtmlPieChart ()
#
# Generates an HTML file which displays a Pie Chart from data in specified CSV file
#
# ARG1 : CSV file to grab data
# ARG2 : Name of desired output HTML file
# ARG3 : Title of the chart
# ARG4 : Position of the chart
# ARG5 : Color scheme
# ARG6 : Flags
# ARG7 : Threshold for flags
# ARG8 : Src folder filepath

sub genHtmlPieChart {
(my $input, my $output, my $title, my $position, my $color, my $flag, my $threshold, my $src) = @_;

my %realData = ();
my $hasFlag = 0;
my $data = "";
my $divText = "";
my $colors = "['#543005','#8c510a','#bf812d','#dfc27d','#f6e8c3','#f5f5f5','#c7eae5','#80cdc1','#35978f','#01665e','#003c30']";

# Read data from CSV file
open (my $inputFile, $input) or die "Could not open $input: $!";

my $total = makeThousands("Total: " . );

# Parse elements and format for javascript / html
while (my $line = ) {
chomp $line;
my @values = split (",", $line);
$realData{$values[0]} = $values[1];
$data .= "\n['$values[0]',$values[1]],";
}

close ($inputFile);

# Parse flags and threshold strings
if (defined $flag and defined $threshold) {
my @flags = split ("\/", $flag);
my

Solution

It's pretty nice! This is readable and good Perl code (if such ever exists). I have only minor suggestions about coding style.

Instead of:

(my $input, my $output, my $title) = @_;


Simpler this way:

my ($input, $output, $title) = @_;


Instead of:

my @values = split (",", $line);
$realData{$values[0]} = $values[1];
$data .= "\n['$values[0]',$values[1]],";


You could simplify:

my ($key, $value) = split (/,/, $line);
$realData{$key} = $value;
$data .= "\n['$key',$value],";


You could replace $line with $_ in loops like this:

while (my $line = ) {
    chomp $line;
    my @values = split (",", $line);
    $schemes{$1} = $2 if ($line =~ /^(\w+):\s*(.*)/);
    # ...
}


Simpler:

while () {
    chomp;
    my @values = split /,/;
    $schemes{$1} = $2 if /^(\w+):\s*(.*)/;
    # ...
}


You could simplify some conditional expressions:

$x = ($hasFlag == 1) ? $y : $z;
if (keys %matches > 0) { ... }


As:

$x = $hasFlag ? $y : $z;
if (keys %matches) { ... }


It's just a matter of taste, but I like to name filehandles with $fh or suffix with fh. So instead of $inputFile, I'd use $inputfh to clarify it's a filehandle.

Code Snippets

(my $input, my $output, my $title) = @_;
my ($input, $output, $title) = @_;
my @values = split (",", $line);
$realData{$values[0]} = $values[1];
$data .= "\n['$values[0]',$values[1]],";
my ($key, $value) = split (/,/, $line);
$realData{$key} = $value;
$data .= "\n['$key',$value],";
while (my $line = <$inputFile>) {
    chomp $line;
    my @values = split (",", $line);
    $schemes{$1} = $2 if ($line =~ /^(\w+):\s*(.*)/);
    # ...
}

Context

StackExchange Code Review Q#59317, answer score: 5

Revisions (0)

No revisions yet.