6
\$\begingroup\$

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;
\$\endgroup\$
6
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – IanNorton
    Aug 7, 2014 at 6:28
  • \$\begingroup\$ 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. \$\endgroup\$ Aug 7, 2014 at 15:08
  • \$\begingroup\$ Is die the best way to signal an exceptional condition from a sub? \$\endgroup\$
    – Snowbody
    Aug 7, 2014 at 15:29
  • \$\begingroup\$ I don't want the program to continue if it is unable to open the file...do you recommend a better way? \$\endgroup\$ Aug 7, 2014 at 15:36
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Sep 10, 2014 at 21:41

1 Answer 1

5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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! \$\endgroup\$ Aug 7, 2014 at 14:51
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – janos
    Aug 7, 2014 at 15:00

Your Answer

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

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