Reading a config file

Question

I am relatively new to Perl and would like to check that the code I am writing docent have any major flaws in it or any bad practices.

This is a fairly simple sub but one that gets used often. Seems to work OK. Simply reads a text file that has parameters in it set by the user.

Config.txt

#cucm username and password of user associated with phones
username  = bob
password  = bobPass
#subnet details. Use /30 to start with to test 2 phones before entire subnet
subnet = 10.64.97.216/32
#HFS web server details IP and folder
webserver   = http://10.64.164.230/Desktop/
#image file names. Full and Thumbnail size
imgFullSize = screen_F.png
imgThumSize = screen_S.png

Perl sub that reads the file:

sub readConfigFile
{
print $lfh "Reading Config.txt file\n";
#path to the config file
my $configF = "config.txt";
if($DEBUG == 1){print "opening config File\n";}
open(my $configFhandle, '<', $configF) or die "Unable to open file, $!";
my @arrFileData=<$configFhandle>; #Slurp!

#go through each of the lines in the file
for (my $i = 0; $i <= $#arrFileData; ++$i)
{
local $_ = $arrFileData[$i];
if($DEBUG == 1){print "Reading line:$_";}
#check to see if the line is a comment... if it is skip it
if($_ =~ /^#/)
{
if($DEBUG == 1){print "Line is comment. Skiping:$_";}
next;
}

#split the line into the key and value
my @dataKV = split /=/, $_;
#check to see that Key and Value exist ... if not skip it
my $arrSize = @dataKV;
if($arrSize <= 1)
{
if($DEBUG == 1){print "Read Config: Error: No Key AND Value found:$_";}
next;
}
#remove all leading and end white spaces around elements
s{^\s+|\s+$}{}g foreach @dataKV;

for (my $x = 0; $x <= $#dataKV; $x++)
{
if($DEBUG == 1){print "Key:$dataKV[$x]\n";}
given($dataKV[$x])
{
when (/^username/) 
{
$usr = $dataKV[++$x];
if($DEBUG == 1){print "Value Username:$dataKV[$x]\n";}
next; 
}
when (/^password/) 
{
$pass = $dataKV[++$x];
if($DEBUG == 1){print "Value password:$dataKV[$x]\n";}
next; 
}
when (/^subnet/) 
{
@arrSubnets = $dataKV[++$x];
if($DEBUG == 1){print "Value Subnet:$dataKV[$x]\n";}
next; 
}
when (/^webserver/) 
{ 
$webserver = $dataKV[++$x];
if($DEBUG == 1){print "Value webserver:$dataKV[$x]\n";}
next;
}
when (/^imgFullSize/) 
{ 
$imgFull = $dataKV[++$x];
if($DEBUG == 1){print "Value imgFullSize:$dataKV[$x]\n";}
next
}
when (/^imgThumSize/) 
{ 
$imgThum = $dataKV[++$x];
if($DEBUG == 1){print "Value imgThumSize:$dataKV[$x]\n";}
next
}
}#end of switch statement           
}#end of for loop going through the key and value line
}#end of For loop going though the lines in the config

print "*********************************\n";
print "*Config imported\n";
print "*********************************\n";
print "*username    :$usr\n";
print "*password    :$pass\n";
print "*subnet      :@arrSubnets\n";
print "*webserver   :$webserver\n";
print "*imgFullSize :$imgFull\n";
print "*imgThumSize :$imgThum\n";
print "*********************************\n";
print "Is this above correct?[Y]:";
my $confirm = <>;
if($confirm !~ /^Y/)
{
print $lfh "Config file needs to be changed. Closing script down.\n";
close($lfh) or warn "Unable to close the file handle: $!";
close($configFhandle) or warn "Unable to close the file handle: $!";
print "Script will now terminate. Please amend \"Config.txt\" file and re-run script. Press     any key to terminate";
<>;
exit;
}
#close the file
close($configFhandle) or warn "Unable to close the file handle: $!";
print $lfh "Y pressed by User. Config all good.\n";
}

Any advise on how to improve this would be great.

Answer 1

Well, here is how I'd write that subroutine. I thoroughly commented the code to explain my choices.

use strict;
use warnings;
use autodie;     # automatic error handling for "open" and other builtins
use IO::Handle;  # object oriented syntax for filehandles

# DEBUG is a compile-time constant initialized by an environment variable.
# If it is false, the debug statements will get optimized away.
use constant DEBUG => $ENV{DEBUG};

# the log file handle and debug file handle
# Here, I opened both to STDERR
open my $lfh, '>&', STDERR;
open my $dfh, '>&', STDERR;

# close the $lfh file handle regardless of how the script terminates.
END {
    no autodie;
    close $lfh or warn "Unable to close the log file handle: $!";
}

# the main part of our script
{
    my $filename = "config.txt";
    # "read_config_file" is a function that takes one argument and returns one value.
    my $config = read_config_file($filename);
    # if the check fails, exit this script with error code "1".
    # the default error code (zero) is considered a success.
    ui_check_config_ok(\*STDOUT, $config, $filename) or exit 1;

    # now we can do stuff with the $config, e.g.
    my $usr = $config->{username};
}

# Name your variables in snake_case, not with camelCase.
# This function *only* parses the config file.
# It does not interact with the user.
# Such separation of concern makes your code easier to maintain:
# every function should do one thing only.
sub read_config_file {
    # Subroutines can take an argument list.
    # We unpack it like this:
    my ($filename) = @_;

    $lfh->say("Reading config file $filename");
    DEBUG and $dfh->say("Opening config file $filename");

    my %known_keys = map { $_ => 1 } qw/
        username password subnet webserver imgFullSize imgThumSize
    /;

    # we wills store the config in this hash
    my %config;

    open my $fh, "<", $filename;

    # don't slurp the file, we loop over it line by line
    LINE:
    while (<$fh>) {
        # trim the line
        s{\A\s+}{};
        s{\s+\z}{};

        DEBUG and $dfh->say("Reading line: $_");
        if (/^#/) {
            DEBUG and $dfh->say("Line is comment, skipping.");
            next LINE;
        }

        # * We use the /x flag on regexes to be able to structure the regex with spaces
        #   they won't match, they're just decoration
        # * instead of assigning to an array, we assign to a list of scalars.
        #   If the "split" fails, the last one should be "undef"
        # * We only accept one "=" per line, and produce 2 fragments max. So
        #       foo = bar = baz
        #   will produce $key="foo", $value="bar = baz".
        my ($key, $value) = split m{\s* [=] \s*}x, $_, 2;
        if (not defined $value) {
            DEBUG and $dfh->say("Ignoring config error: no key = value pair found on line: $_");
            next LINE;
        }

        DEBUG and $dfh->say("Value $key:$value");
        $config{$key} = $value;

        # a sanity check – you don't do anything on unknown keys
        if (not exists $known_keys{$key}) {
            $lfh->say("The config provided the unknown key $key, ignoring.")
        }
    }

    # another sanity check – did the user forget to specify some properties?
    for my $key (keys %known_keys) {
        if (not exists $config{$key}) {
            $lfh->say("The config didn't provide a key $key, ignoring.")
        }
    }

    return \%config;
}

# another subroutine that provides the user interface to ask whether the data is correct
# It will return a truth value whether the config was OK.
sub ui_check_config_ok {
    # Here, we see an argument list with three values
    my ($ui_fh, $config, $filename) = @_;

    $ui_fh->say($_) for
        "*********************************",
        "*Config imported",
        "*********************************",
        "*username    :$config->{username}",
        "*password    :$config->{password}",
        "*subnet      :$config->{subnet}",
        "*webserver   :$config->{webserver}",
        "*imgFullSize :$config->{imgFullSize}",
        "*imgThumSize :$config->{imgThumSize}",
        "*********************************";
    $ui_fh->print("Is this above correct?[Y]:");
    if (<STDIN> =~ /^\s*Y/) {
        $lfh->say("The user pressed Y. Config is good.");
        return 1;
    }

    $lfh->say("Config file contains false information, please correct.");
    $lfh->say("Terminating script");
    $ui_fh->say("The script will now terminate.");
    $ui_fh->say("Please amend the file $filename and rerun the script.");
    $ui_fh->say("Press any key to terminate...");
    <>;
    return 0;
}

Note how indentation made this code much easier to read than the dump you provided. Please use indentation!

I actually do not generally use IO::Handle, but it makes stuff more obvious when having more than one file handle. The autodie module is a bit inflexible, but I highly encourage it when you are still new with Perl, as it provides sensible defaults. You do not manually have to close file handles if they are variables declared with my – they will get closed automatically, and it's highly unlikely that closing a file will fail (file handles are not only used for physical files, but also for other things like pipes, where the close can fail).