need constructive critism for my perl script

simuru · 2010-10-10 19:59:27

Hi there,
today i dugg in a bit of perl and decided to write a script for my scanner.
Since i'm brand new to perl i would like you to comment and critizise my code so i can improve it.

The code:

#!/usr/bin/perl
use Term::Size;
use Term::ANSIColor;
use Cwd;

# ------------------------------------------------------------------------------------------ #
# Configuration options
# Everything will be asked by the program when run; but you can hardcode it of course.
$CONFIG_STATUS = 0;   # Is the configuration corret? Dont know yet if it can fail....
$USER_FILENAME = "y";    # Does the user want to name the files by himself?
$USER_SCANSIZE = "y";    # Does the user want to specify the size for every scan?
$DEFAULT_X     = 210;  # Default x-size; 210mm is equal to din A4 format paper
$DEFAULT_Y     = 270;  # Default y-size; 270mm is equal to din A4 format paper
$COMPRESSION   = 0;    # 0 means no compression, 1 means jpeg, 2 means zip compression method
$COMPRESSION_QUALITY = 50;
$TIFF_OUTPUT   = getcwd . "/tiff/";  # The default input folder for the *.tiff files
$PDF_OUTPUT    = getcwd . "/pdf/";   # The default outfolder for the *.pdf files
$KEEP_TIFF     = 0;                  # 0: Do not Keep Tiff files
$SCANNER_ADDR              = "foobar";
$SCANNER_DEV_ADDR          = "/dev/scanner0";    # Default scanner - probably not reachable
$SCANNER_VENDOR            = "vendor id";        # Vendor of the scanner
$SCANNER_PRODUCT_ID        = "device id";    # device id of scanner
$VERBOSE                   = 0;              # Should we be verbose?
$NUMBER_SCANS              = 0;
$SCANS_DONE = 0;
# ------------------------------------------------------------------------------------------ #

sub scan {
    if($NUMBER_SCANS != 0 && $SCANS_DONE == $NUMBER_SCANS){
        # TODO: Call cleanup function if necessary
    }
    if($USER_FILENAME == "y"){
        print "Please enter filename: ";
        $FILE_NAME_SN = <STDIN>;
        chomp($FILE_NAME_SN);
        $FILE_NAME_SN = $FILE_NAME_SN . ".tiff";
    } else {
        $FILE_NAME_SN = "Scan" . $SCANS_DONE . ".tiff";
    }
    chomp($USER_SCANSIZE);
    if($USER_SCANSIZE == "n"){
        print "Please enter breadth(in mm): ";
        $DEFAULT_X == <STDIN>;
        print "Please enter width(in mm): ";
        $DEFAULT_Y == <STDIN>;
    }
   
    chomp($SCANNER_ADDR);
    chomp($FILE_NAME_SN);
    print "scanimage -d $SCANNER_ADDR -x $DEFAULT_X -y $DEFAULT_Y --progress --format=tiff > $TIFF_OUTPUT$FILE_NAME_SN \n";
    system("scanimage -d $SCANNER_ADDR -x $DEFAULT_X -y $DEFAULT_Y --progress --format=tiff > $TIFF_OUTPUT$FILE_NAME_SN");
    $SCANS_DONE += 1;
}

sub convert {
    $TERM_SIZE = Term::Size::chars *STDOUT{IO};
    printf "." x $TERM_SIZE . "\n";
    @files = <tiff/*.tiff>;
    foreach $file (@files) {
        $in = $file;
        $file =~ s/\..*//;
        $file = substr($file, 5);
        $file_name = "pdf/" . $file . ".pdf";
        print color 'bold blue';
        print "converting ";
        print color 'red';
        print $in;
        print color 'reset';
        print " to ";
        print color 'green';
        print "$file_name";
        system("tiff2pdf -p A4 -o $file_name -j $in");
        if($? == -1){
            $len = 20 + length($in) + length($file_name);
            print color 'red';
            print " " x ($TERM_SIZE - $len) . " Fail\n";
        } else{
            $len = 20 + length($in) + length($file_name);
            print " " x ($TERM_SIZE - $len) . " Done\n";
        }
    }
}

sub get_config {
    # Be vebose?
    print "Is verbose output needed? (y/n): ";
    $VERBOSE = <STDIN>;

    # Is number of scans known already?
    do {
        print "Please enter number of scans [default = user will be prompted]: ";
        $NUMBER_SCANS = <STDIN>;
    } while ($NUMBER_SCANS < 0);
    
   # Is the paper size always the same?
   print "Always use the same paper size? (y/n): ";
   $USER_SCANSIZE = <STDIN>;

   # Should the *.tiff files be kept?
   print "Save the .tiff files(uncompressed)? (y/n): ";
   $KEEP_TIFF = <STDIN>;

   # Chose compression method
   print "Do you want the .pdf files to be compressed?\n\t",
         "0 = no compression\n\t",
         "1 = jpg compression\n\t",
         "2 = zip compression\n\t";
   $COMPRESSION = <STDIN>;

   if($COMPRESSION == 1){
       print "Please enter compression quality for jpeg [1 - 100]: ";
       $COMPRESSION_QUALITY = <STDIN>;
   }

   # default folders for .tiff / .pdf files
   if($KEEP_TIFF == "y"){
       print "Where should the .tiff files be saved? [default: " . $TIFF_OUTPUT . " ]\n\t: ";
       $TMP = <STDIN>;
       if($TMP != "\n"){
           $TIFF_OUTPUT = $TMP;
       }
   }

   print "Where should the .pdf files be saved? [default: " . $PDF_OUTPUT . " ]\n\t: ";
   $TMP = <STDIN>;
   if($TMP != "\n"){
       $PDF_OUTPUT = $TMP;
   }

   print "Do you want to name the files manually? [default: 001.pdf .. xxx.pdf] (y/n): ";
   $USER_FILENAME = <STDIN>;
}

sub get_scanner {
    # Check if Sane is installed ?
    @sane_output = `sane-find-scanner`;
    foreach $line (@sane_output){
        if($line =~ m/vendor/){ # Simple but it works
            $SCANNER_VENDOR = $line;
            $SCANNER_DEV_ADDR = $line;
            $SCANNER_PRODUCT_ID = $line;
            $SCANNER_VENDOR =~ s/.*vendor=//;
            $SCANNER_VENDOR =~ s/, product.*//;
            $SCANNER_DEV_ADDR =~ s/.*at //;
            $SCANNER_PRODUCT_ID =~ s/.*product=//;
            $SCANNER_PRODUCT_ID =~ s/, chip.*//;
        }
    }
    @scanimage_output = `scanimage -L`;
    if($scanimage_output[1] =~m/No.*/){
        goto DIE;
    } else {
        if($scanimage_output[0] =~m/device.*/){
            print $scanimage_output[0];
            $SCANNER_ADDR = $scanimage_output[0];
            $SCANNER_ADDR =~ s/.*device `//;
            $SCANNER_ADDR =~ s/..is.*//;
            goto FOUND_DEVICE;
        }
    }
    DIE:
        die "Sane was not able to Detect a Scanner.\n";

    FOUND_DEVICE: 
}

# Beginning of main execution;
if($CONFIG_STATUS == 0){
    $SCANS_DONE = 0;
    get_scanner;
    if($VERBOSE == "y"){
            print "Scanner Vendor: " . $SCANNER_VENDOR, 
                  "Scanner Device Adress: " . $SCANNER_DEV_ADDR, 
                  "Scanner Product Id: " . $SCANNER_PRODUCT_ID,
                  "Scanner Sane Adress: " . $SCANNER_ADDR;
    }
    get_config;
    scan;
    #convert;
} else {
    print "Config Failed\n";
    print_config;
}

Thanks and a happy 10/10/10.

juster · 2010-10-12 20:30:17

One of the cliched cardinal rules of learning perl programming is to "always use strict and use warnings". These basically keep you from shooting yourself in the foot.

use strict requires you to explicitly define your variables before using them. Right now your variables are defined as package variables and created as soon as they are used. If you started to use strict; you would have to prefix your variables with my or our before using them. Since you have predefined them all in the top this isn't too much of a problem.

Global variables are generally not encouraged and I would recommend passing parameters to your subs and returning hashes or hash references from them. Why? Because it is harder to track how global variables get set and used later, in what order, etc. Some global variables don't seem to need to be global like $KEEP_TIFF for instance. These are only used inside the get_config sub. As a style guide I like capitalizing global variables so they stand out but you also have variables used only by single subroutine (like $TERM_SIZE inside the convert sub) that are capitalized as well. This is confusing.

Remember with use strict; you have to declare variables explicitly. You would have my $term_size inside the sub where you use it, basically saying... hey I'm only using $term_size here don't worry about it cropping up elsewhere. Variables declared with my are lexically defined, which means they are only visible inside their current scope, scope is defined with brackets and can be nested. If you are inside brackets looking out this works but if you are outside trying to peek in you won't be able to. Scoping is a handy way to indicate the variable is only used inside a subroutine, removing the burden on the reader to remember it for later in case it happens to be used elsewhere.

my $foo = 'Hello World!';

sub test
{
    my $bar = 'Goodbye, cruel World!';
    print "$foo\n$bar\n";
}

test();
print "$foo\n$bar\n"; # fails

use warnings; prints a warning out whenever you do something that is a rather common mistake. Warnings don't stop the program like strict does. If you do add use warnings to your script you will see a few mistakes you have made. Perl has separate relational operators for numbers (==, <, >, !=) and strings (eq, ne, lt, gt). When you use a numeric operator on a string unexpected things occur, like it always returns true. If the string represents a number, like "100", then it is automatically converted, but otherwise this is a bad practice. use warnings will warn you when this happens.

So much of your code has prompts in it that it would be helpful to use some sort of function that will read prompts and chomp it for you, something like:

sub prompt
{
    my ($question) = @_;
    chomp $question;
    print $question;
    my $answer = <STDIN>;
    chomp $answer;
    return $answer;
}

Or better yet you could use a module that will check for valid user input as well. Something like Term::Prompt. You might also consider using command-line options instead of prompts by using the Getopt::Long module.

I hope that's the type of criticism you wanted. For a first script I think it's a good job!

juster

Daenyth · 2010-10-13 15:47:54

What Juster said about strict;warnings; is totally correct, and you will never want to go without them in your perl career. There's not a ton more to add to that, so I'll address other things.

First, I'd change your variable names. Instead of having a cryptic variable name with a comment to explain it, name the variable better.

For example,

$CONFIG_STATUS = 0;   # Is the configuration corret? Dont know yet if it can fail....

will become

my $CONFIG_IS_CORRECT = 0; # Don't know if this can fail

.

Also, you should stick to 1/0 for true/false. Using strings like "y" is very bad practice.
You can also clean up your code with some hashes to store related values.

For example,

$DEFAULT_X     = 210;  # Default x-size; 210mm is equal to din A4 format paper
$DEFAULT_Y     = 270;  # Default y-size; 270mm is equal to din A4 format paper
$COMPRESSION   = 0;    # 0 means no compression, 1 means jpeg, 2 means zip compression method

will become

my %PAPER_SIZES = ( 'A4' => { 'x' => 210, 'y' => 270 }, ); # Paper x,y sizes are in mm
my $DEFAULT_SIZE = 'A4';
my %COMPRESSION_TYPES = ( 'none' => 0, 'jpeg' => 1, 'zip' => 2 );
my $DEFAULT_COMPRESSION = 'none';

Hope this helps.

sujoy · 2010-10-17 03:17:18

to setup things like paper size, verbose or not, no. of scans, where to store the files, etc ... you should ideally have command line arguments.
for ex,
-v
-n [number of scans]

and so on. this would improve the overall usability of the program.

cheers.

Arch Linux

#1 2010-10-10 19:59:27

need constructive critism for my perl script

#2 2010-10-12 20:30:17

Re: need constructive critism for my perl script

#3 2010-10-13 15:47:54

Re: need constructive critism for my perl script

#4 2010-10-17 03:17:18

Re: need constructive critism for my perl script

Board footer