Re: Defactor this code

I would have written that process as follows:

use strict;

die "Usage: $0 port#\n (where port# is a number > 9 and < 65536\n"
    if ( @ARGV != 1 or $ARGV[0] !~ /^[1-9]\d+$/ or $ARGV[0] >= 65536 )
+;

my $sshport = shift;
my $filename = "sshd_config";

open (SSHD_CONFIG,$filename)
    or die ("can't open $filename for reading: $!");
open (SSHD_CONFIG_NEW,">","$filename.new")
    or die ("can't open $filename.new for writing: $!");

while (<SSHD_CONFIG>){
    s/^(#?)port.+/$1Port $sshport/i;
    print SSHD_CONFIG_NEW;
}
( rename $filename, "$filename.old" and
  rename "$filename.new", $filename )
    or die "rename failed: $!"
[download]

(updated to fix the tests on $ARGV[0])

The points I would make about this, relative to the OP version:

Using command-line args and @ARGV is more effective, more flexible, less tedious, and just better than prompting for interactive input from STDIN -- both for the programmer and for the user (e.g. using @ARGV this way means you can run the script as a non-interactive process in a cron job, a pipeline command, etc).
The OP version checks each input line to see if it matches either "Port" or "port" at the beginning of the line (possibly preceded by "#"), but then it does a substitution only if the line has "Port", which is strange; the "if" match condition should have been the same as the match condition for the s/// (in fact, as mentioned earlier, you only need the s///).
The OP version has a side effect of changing "#Port" to "Port"; this also seems strange, and probably a bug. Maybe I'm wrong, and should not assume that it's a bug, but I think the burden would be on you to make it clear why this should not be considered a bug.
Anytime you find yourself using the same string or numeric literal (e.g. a file name) more than twice and/or at very different locations in the script, you'll probably be better off assigning that value to a variable (or use constant); if you ever need to change the literal value in the future, it'll be better to have to change it at just one point in the script.

Comment on Re: Defactor this code Select or Download Code

Replies are listed 'Best First'.
Re^2: Defactor this code by xerophyte (Initiate) on Feb 10, 2007 at 20:29 UTC
Awesome, i feel learned lot fro this defector, i have one problem the sshd_config sometime looks like this #Port 22 Port 4444 #Protocol 2,1 #ListenAddress 0.0.0.0 #ListenAddress :: After we modify the file with above code it becomes like this Port 4444 Port 4444 it should be lie Port 4444 Thanks again	[reply]
Re^3: Defactor this code by graff (Chancellor) on Feb 11, 2007 at 06:27 UTC
i have one problem the sshd_config sometime looks like this `#Port 22 Port 4444` [download] After we modify the file with above code it becomes like this `Port 4444 Port 4444` [download] Right, that's what your code in the OP does, and that's what I mentioned in my third "bullet" paragraph as probably being a bug. Now, you seem to be saying that you simply want to delete all commented lines (the ones beginning with "#"), which is easy enough to do: `while (<SSHD_CONFIG>) { next if ( /^#/ ); s/^port.+/Port $sshport/i; print SSHD_CONFIG_NEW; }` [download] But some people prefer to leave the comment lines in, because sometimes these lines hold important information that might be useful someday (e.g. documentation, or details about a prior or alternative configuration that might be needed when conditions change). That's why the version in my first reply used the parens and the "$1" in the s/// statement, to preserve the "#" character while changing the port number. Actually, I think it may be best to retain the comment lines unchanged, and only edit the "uncommented" line -- this also turns out to be the simplest method: `while (<SSHD_CONFIG>) { s/^port.+/Port $sshport/i; print SSHD_CONFIG_NEW; }` [download]	[reply] [d/l] [select]