http://www.perlmonks.org?node_id=85378


in reply to netswap.pl

malloc asked for comments about his style/coding practices, and I saw his useful netswap.pl program as a good exercise for a code/style review (not golf!).
I'm not claiming that the proposed code is perfect nor the original code is bad (in fact, I really like the use of the -i switch to edit files in a convenient way in a program), but I hope to give some hints on how can one use perlish features to improve code.

  • Use strict and -w are personnaly a must when I have to write programs that are either more than 10 lines long or are to be used more than 10 times (I guess I first read that in one of merlyn's column)

    #!/usr/bin/perl -wi.bak
use strict;

    [download]

  • I replaced the bunch of scalars by a hash. It will be easier to add other keys if we have to process other files in the future.
    Also note that you could have declared several my variables with something like "my ($var1, $var2, $var3);".

    my %params;
my $filename = shift || die "Usage: netswap.pl CONFILENAME\n";

    [download]

  • I added a meaningful error message by appending the "$!" variable, to indicate precisely what the problem is.

    open(FH,"$filename")
    or die "Could not open network configuration file: [$filename] for
+ reading: $!\n";

    [download]

  • I got rid of the "$line = $_", as the $line scalar was not really useful for future operations.
    The key/value extracted are stored in a hash, which is I think the easiest and safest way to do.
    Using "soft references" as you did (${$param_name} = $param_val;) might cause some problems (for instance, what happens if you extract a key named 'value'?). 

    while(<FH>){
    # Skip the blank lines or the commented lines
    next if (/^\s*$/ || /^\s*\#/);

    # Get the name and the values
    my ($name, $value) = split(/:/, $_, 2);

    # Remove starting/trailing spaces    
    s/(?:^\s*|\s*$)//g foreach ($name, $value);

    # Store the values in the hash in lowercase to 
    # avoid typing mistakes
    $params{$name} = lc($value);
}

    [download]

  • Mostly cosmetic choice here, I did a here-document instead of printing a several lines long string. I just happen to find here-docs more readable.

    print <<"EOL";
new hostname    = [$params{hostname}]
new domainname  = [$params{domainname}]
new ip          = [$params{ip}]
new Subnet Mask = [$params{subnetmask}]
new DNS         = [$params{dns}]
new gateway     = [$params{gateway}]

EOL

    [download]

  • Now, here's maybe my most criticizable modification. Instead of calling several times your file_edit function, I stuffed a big hash reference with all the infos on the replacements to occur in the file.
    The filenames are the keys of a second hash with the regex to modify as key and the modified string as value. 

    my $replacements = {
    '/etc/hosts' => {
    'hosts' => {
        "^.*$params{hostname}.*" => "$params{ip}\t$params{hostname}.$p
+arams{domainname}\t$params{hostname}",
    },

    '/etc/sysconfig/network' =>    {
        "^GATEWAY.*"      => qq'GATEWAY="$params{gateway}"',
        "^HOSTNAME.*"     => qq'HOSTNAME="$params{hostname}"',
        "^DOMAINNAME.*"   => qq'DOMAINNAME="$params{domainname}"',
    },

    '/etc/sysconfig/network-scripts/ifcfg-eth0' => {
        "^IPADDR.*"       => qq'IPADDR="$params{ip}"',
        "^NETMASK.*"      => qq'NETMASK="$params{subnetmask}"',
    },

    '/etc/resolv.conf' => {
        "^nameserver.*"   => "nameserver $params{dns}",
        "^SEARCH.*"       => "SEARCH $params{domainname}",
    },
};

file_edit($replacements);

`/etc/init.d/network restart`;

########################file_edit###################

sub file_edit {
    my $replacements = shift;
    foreach my $filename (keys %{$replacements}){
        print STDERR "Processing file [$filename]\n";

    [download]

  • Instead of harshly dying, I chose print an error and skip to the next file, if the file doesn't exists.

            unless (-e $filename){
            print STDERR "  [$filename] cannot be found!\n";
            next;
        }

    [download]

  • I really like the use of the -i switch to modify inplace the files! However I added the .bak suffix, just in case something goes wrong.

            @ARGV=($filename); # put the filename where perl can find it.
        while(<>){ 
            foreach my $search (keys %{$replacements->{$filename}}){
                s/$search/$replacements->{$filename}->{$search}/;
                print;
            }
        }
        print STDERR "  done\n";
    }
}

    [download]

    For better advices and hints on how to improve one's code and style, I strongly suggest reading Dominus most excellent "Program Repair Shop and Red Flags" series available at Dominus' homepage.

    Now, I'd really like to get some coding/style review on the code above!

    If everything goes well, you should get the whole new code in one piece if you press d/l code!

    <kbd>--
    my $OeufMayo = new PerlMonger::Paris({http => 'paris.mongueurs.net'});</kbd>

In Section Code Catacombs