Readability of this Perl script to generate HTML code

Question 1

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: " . <$inputFile>);
 # Parse elements and format for javascript / html
 while (my $line = <$inputFile>) {
 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 @thresholds = split ("\/", $threshold); 
 my %compareData = ();
 @compareData{@flags} = @thresholds;
 # Determine if any real data entry meets threshold for flag
 my %matches = ();
 foreach my $compare (keys %compareData) {
 foreach my $real (keys %realData) {
 if (lc $compare eq lc $real and $realData{$real} >= $compareData{$compare}) {
 $matches{$real} = $realData{$real};
 $hasFlag = 1;
 }
 }
 }
 # Create flag div if matches found
 if (keys %matches > 0) {
 $divText .= "<div id=\"flag\"><table>\n";
 $divText .= "<tr><td>$_</td><td>" . makeThousands($matches{$_}) . "</td></tr>\n" foreach (keys %matches);
 $divText .= "</table></div>\n";
 }
 }
 # Set border width and padding according to position of chart
 my %positionProperties = (
 "TR" => ['border-width: 10px 10px 5px 5px;','bottom: -2px; left: 6px;','bottom: -2px; right: 12px;'], # Top Right
 "BR" => ['border-width: 5px 10px 10px 5px;','bottom: 4px; left: 6px;','bottom: 4px; right: 12px;'], # Bottom Right
 "BL" => ['border-width: 5px 5px 10px 10px;','bottom: 2px; left: 12px;','bottom: 2px; right: 6px;'], # Bottom Left
 "TL" => ['border-width: 10px 5px 5px 10px;','bottom: -2px; left: 12px;','bottom: -2px; right: 6px;'] # Top Left
 );
 my $chartBorders = $positionProperties{$position}->[0];
 my $flagBorders = $positionProperties{$position}->[1];
 my $totalBorders = ($hasFlag == 1)?$positionProperties{$position}->[1]:$positionProperties{$position}->[2];
 # Set color scheme according to 'color' argument
 open (my $colorsFile, $src . "colors.txt") or die "Could not open colors.txt: $!";
 my %schemes = ();
 while (my $line = <$colorsFile>) {
 chomp $line;
 $schemes{1ドル} = 2ドル if ($line =~ /^(\w+):\s*(.*)/);
 }
 $colors = $schemes{$color} if (defined $schemes{$color});
 close ($colorsFile);
 # Write HTML and Javascript to file
 open (my $outputFile, '>', $output) or die "Could not open $output: $!";
 print $outputFile qq~
 <!DOCTYPE HTML>
 <html>
 <head>
 <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
 <meta http-equiv="refresh" content="3600">
 <title>$title</title>
 <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js"></script>
 <link href='http://fonts.googleapis.com/css?family=Oswald:400' rel='stylesheet' type='text/css'>
 <script type="text/javascript">
 var ofs = 0;
 window.setInterval(function(){
 \$('#flag').css('background', 'rgba(214,49,71,'+Math.abs(Math.sin(ofs))+')');
 ofs += 0.01;
 }, 20);
 \$(function () {
 Highcharts.setOptions({
 colors: $colors,
 						lang: {
 							thousandsSep: ',',
 						}
 });
 var chart;
 \$(document).ready(function () {
 \$('#chart').highcharts({
 chart: {
 type: 'pie',
 plotBackgroundColor: null,
 plotBorderWidth: null,
 plotShadow: false,
 backgroundColor: '#f5f5f5'
 },
 title: {
 text: '$title',
 style: {
 fontSize: '35px',
 fontWeight: 'bold',
 fontFamily: 'Oswald'
 }
 },
 tooltip: {
 pointFormat: '{series.name}: <b>{point.percentage:.1f}%</b>'
 },
 plotOptions: {
 pie: {
 allowPointSelect: true,
 cursor: 'pointer',
 dataLabels: {
 enabled: true,
 format: '<b>{point.y:,.0f}</b>: {point.percentage:.1f} %',
 style: {
 color: (Highcharts.theme && Highcharts.theme.contrastTextColor) || 'black',
 fontSize: '15px'
 }
 },
 showInLegend: true
 }
 },
 legend: {
 layout: 'vertical',
 align: 'right',
 verticalAlign: 'top',
 y: 50,
 backgroundColor: '#e4e4e4',
 borderRadius: '5px',
 itemStyle: {
 fontSize: '15px',
 fontWeight: 'light',
 width: 140
 },
 itemWidth: 200
 },
 credits: {
 enabled: false
 },
 series: [{
 name: '$title',
 innerSize: '40%',
 data: [ $data ]
 }]
 });
 });
 });
 </script>
 <style type="text/css">
 #container {
 position: relative;
 width: 945px; 
 height: 525px; 
 }
 #chart {
 width: 100%; 
 height: 100%; 
 margin: 0 auto; 
 border-color: #333; 
 border-style: solid; 
 margin: -8px 0 0 -8px;
 $chartBorders
 }
 #flag {
 border-radius: 5px;
 padding: 10px;
 color: #fff;
 z-index: 1000; 
 position: absolute; 
 $flagBorders
 }
 #total {
 border-radius: 5px;
 padding: 10px;
 color: #fff;
 z-index: 1000; 
 position: absolute;
 font-family: Oswald;
 font-size: 1.5em;
 background-color: #333;
 padding: 5px 10px 5px 10px;
 $totalBorders
 }
 table {
 table-layout: fixed;
 width: 200px;
 }
 table tr td{
 text-align: left;
 padding-left: 5px;
 padding-right: 5px;
 word-wrap: break-word;
 font-family: Oswald;
 font-size: 1em;
 }
 table tr td:nth-child(2) {
 text-align: center;
 font-size: 1.5em;
 }
 </style>
 </head>
 <body>
 <script src="http://code.highcharts.com/highcharts.js"></script>
 <div id="container">
 <div id="chart"></div>
 $divText
 <div id="total">$total</div>
 </div>
 </body>
 </html>
 ~;
 close $outputFile;
}
1;

Question 2

My initial instinct is to have two files, one being a sort of template file containing the bulk of the javascript with a placeholder that your perl script can insert the input data for the chart. I think the perl would get much simpler then. I'd also be a little tempted to do as much of the HTML table manipulation in javascript as possible.

Question 3

I'll definitely put some thought into that. Unfortunately I'm not too familiar with javascript which is why I did the tables in HTML, but I'll look into it.

Question 4

Is die the best way to signal an exceptional condition from a sub?

Question 5

I don't want the program to continue if it is unable to open the file...do you recommend a better way?

Question 6

Generating HTML by hard-coding it in your script makes for a maintenance nightmare down the road. That's what templating systems were designed for. Take a look at Template Toolkit, for example.

Question 7

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 = <$inputFile>) {
 chomp $line;
 my @values = split (",", $line);
 $schemes{1ドル} = 2ドル if ($line =~ /^(\w+):\s*(.*)/);
 # ...
}

Simpler:

while (<$inputFile>) {
 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.

Question 8

Thanks! $colors is actually used; it is down in the Javascript...I like the shortcut for making it an array of strings, but it needs to be a string of an array of strings (if that makes sense). I don't think it would be interpolated as so if it were just an array of strings. I appreciate the review!

Question 9

I meant, you're overwriting it somewhere in the middle: $colors = $schemes{$color} if (defined $schemes{$color}). But I'm wrong, it depends on the if, and you might need the default value. I'll drop that point from the review.

janos janos 113k15 gold badges154 silver badges396 bronze badges · Accepted Answer · 2014-08-07 05:43:47Z

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 = <$inputFile>) {
 chomp $line;
 my @values = split (",", $line);
 $schemes{1ドル} = 2ドル if ($line =~ /^(\w+):\s*(.*)/);
 # ...
}

Simpler:

while (<$inputFile>) {
 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.

Thanks! $colors is actually used; it is down in the Javascript...I like the shortcut for making it an array of strings, but it needs to be a string of an array of strings (if that makes sense). I don't think it would be interpolated as so if it were just an array of strings. I appreciate the review!
I meant, you're overwriting it somewhere in the middle: $colors = $schemes{$color} if (defined $schemes{$color}). But I'm wrong, it depends on the if, and you might need the default value. I'll drop that point from the review.

Stack Exchange Network

Readability of this Perl script to generate HTML code

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Readability of this Perl script to generate HTML code

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions