Beefy Boxes and Bandwidth Generously Provided by pair Networks
There's more than one way to do things

Comment on

( #3333=superdoc: print w/replies, xml ) Need Help??

malloc asked for comments about his style/coding practices, and I saw his useful 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;
  • 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: CONFILENAME\n";
  • 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";
  • 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); }
  • 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
  • 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";
  • 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; }
  • 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"; } }

    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!

    my $OeufMayo = new PerlMonger::Paris({http => ''});</kbd>

In reply to Re: (style/coding comments) by OeufMayo
in thread by malloc

Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":

  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.
  • Log In?

    What's my password?
    Create A New User
    and all is quiet...

    How do I use this? | Other CB clients
    Other Users?
    Others pondering the Monastery: (7)
    As of 2018-05-22 14:52 GMT
    Find Nodes?
      Voting Booth?