How terrible is my Perl? Script that takes IP addresses and returns Fully Qualified Domain Names

Question

I invite you, tear me a new one.

This code gets the job done. It takes a .txt file containing a list of IPs and writes a file containing their respective fully qualified domain names.

I want to know in what ways is this code poorly written. What bad habits are here?

I am a perl and programming newbie. I managed to put this together using google and trail and error. Getting it to work was satisfying but please tell me how I can improve.

use strict;
use warnings;
use Socket;
use autodie;


my $filename = 'IPsForFQDN.txt';
#File with list of IPs to lookup.
#One IP address per line like so:
#10.10.10.10
#10.10.10.11
#10.10.10.12
#etc...


open(my $fh, '<:encoding(UTF-8)', $filename)
    or die "Could not opne file '$filename' $!";
my $fqdn = '';

while (my $row = <$fh>) {
    chomp $row;

    print "$row\n";
    $fqdn = gethostbyaddr(inet_aton($row), AF_INET);
    print $fqdn;
    print "\n";
    open FILE, ">>fqdn.txt" or die $!;
    print FILE $fqdn;
    print FILE "\n";
    close FILE;

}
print "done\n";

For instance is the {chomp $row;} line needed? I have NO IDEA what it does.

I am equally mystified by the whole {or die $!;} thing.

Answer 1

$! reports why something failed. Here if you were unable to open the file the reason for failure would be pointed out. perlvar has a section on error variables.

You're using chomp to remove the newline character from the end of each line.

When writing the file you call open slightly differently, consider using the same 3 argument version as you do when opening for reading earlier in your code (also see the link I gave you for open), and in the same coding style. It's good to be consistent, also this method is safer.

Answer 2

You're repeatedly opening fqdn.txt for every line you write. I'd just open it before the loop and close it at the end.

Oh - and you're using autodie so the or die shouldn't be necessary.

Oh - and you've used old-style open for it too, compared to new-style open for the reading file.

Answer 3

Not much going on at work so I had a go at a little rewrite with comments in to explain a few things. Not right/not wrong just my spin and a few of the standards we use at my place have been added.

Hope this helps...

use strict;
use warnings;
use Socket;

# initialize variables here.
my $filename = "IPsForFQDN.txt";

# open both file handles - once only
# Note safer expression using 2 commas
open(FH, "<", $filename)
        or die "Could not opne file '$filename' $!";

# open FILE for appending
open FILE, ">>", "fqdn.txt" or die $!;

# use foreach instead of while - easier syntax (may provoke discussion ;-) )
# replaced $fh for FH - use file handles throughout for consitency
foreach my $row ( <FH> )
{
    chomp $row;

    # put a regex check in for comments

    if( $row !~ m/^#/ )
    {
        printf ("Row in file %s \n", $row );

        # initialize $fqdn here to keep it fresh
        my $fqdn = gethostbyaddr(inet_aton($row), AF_INET);

        # formatted print to screen (STDOUT)
        printf ("FQDN %s \n", $fqdn);

        # formatted print to output file
        printf FILE ("%s \n", $fqdn);
    }
}

# close both file handles - once only
close FILE;
close FH;

print "done\n";