Defactor this code

xerophyte has asked for the wisdom of the Perl Monks concerning the following question:

Replies are listed 'Best First'.
Re: Defactor this code by kyle (Abbot) on Feb 10, 2007 at 13:20 UTC
You can write your input loop so that you don't repeat yourself. Also, I think you probably do not want to allow port numbers ten digits long. It's probably best to test whether the number is in an acceptable range. `my $sshport; while ( $sshport !~ /^[1-9]\d+$/ \|\| $sshport > 65535 ) { print " Please Enter valid ssh port number:\n"; chomp( $sshport = <STDIN> ); }` [download] It might be even better to use something like IO::Prompt. It's probably not necessary to test whether the line matches the pattern you want to replace before doing the replacement. If the pattern in the replacement doesn't match, nothing will be done anyway. That said, maybe you really did intend for the replacements to be done in circumstances slightly different than when the replacement matches. The code below behaves a little differently than yours, but I think it's closer to what you meant. `while (<SSHD_CONFIG>){ s/^Port.+/Port $sshport/i; s/^#Port\s.+/Port $sshport/i; print SSHD_CONFIG_NEW $_; }` [download] You could even make those two replacements into one (`s/^(?:Port\|#Port\s).+/Port $sshport/i`). You should check whether rename worked: `rename('sshd_config','sshd_config.old') or die "Can't rename sshd_config: $!"; rename('sshd_config.new','sshd_config') or die "Can't rename sshd_config.new: $!";` [download]	[reply] [d/l] [select]
Re: Defactor this code by japhy (Canon) on Feb 10, 2007 at 13:27 UTC
First, your $errorstate variable is named the opposite of what it should be. You set it to 1 when the input is formatted properly! Then, the bottom half of your code can be idiomized using some built-in variables in Perl. And are you sure you want to REMOVE the '#' from lines starting with '#port'? You also do some unnecessary work of checking to see if a regex matches before doing a substitution using that regex. It turns out you can get rid of $errorstate altogether, and just loop until $sshport has a value. `#!/usr/bin/perl use strict; use warnings; my $sshport = 0; until ($sshport) { print "Please enter SSH port number> "; chomp($sshport = <STDIN>); $sshport = 0 unless $sshport =~ /^[1-9]\d{1,9}$/; } { local $^I = ".old"; # the "in-place edit backup extensio +n" variable local @ARGV = ("sshd_config"); # the files to edit in-place while (<>) { s/^(#?)port ./$1Port $sshport/i; # this KEEPS the '#' character, # which I think is the correct behavior } }` [download] See perlvar for information about $^I. Jeff `japhy` Pinyan, P.L., P.M., P.O.D, X.S.: Perl, regex, and `perl` hacker How can we ever be the sold short or the cheated, we who for every service have long ago been overpaid? ~~ Meister Eckhart*	[reply] [d/l]
Re: Defactor this code by graff (Chancellor) on Feb 10, 2007 at 16:13 UTC
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.	[reply] [d/l] [select]
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]
Re: Defactor this code by smahesh (Pilgrim) on Feb 10, 2007 at 13:37 UTC
Hi xerophyte, I hope this is not homework. Some suggestions that I could quickly come up with are: Use "use strict;" and "use warnings;" in the code (atleast during development). Minor nitpick: Use a better named variable than "$errorstate". Using "$errorstate = 1" to indicate that valid sshport was input is confusing. I would have used a name like "$isvalidport", etc. Instead of reading in the port value from <STDIN>, pass it as a cmdline argument. Port numbers are limited to 65535 max. So, a value larger than that should be treated as invalid data. Instead of using s/.../.../ to modify $_, why not print the new line "Port $sshport" to SSHD_CONFIG_NEW directly? I feel the substition operation is not required. The current print statement can be moved inside an else block. Please close the opened file handles before calling rename(...) You should check the return status of the rename(...) command. Regards, Mahesh	[reply]
Re: Defactor this code by sfink (Deacon) on Feb 11, 2007 at 04:57 UTC
For things like this, it's nice to be familiar with some of the more esoteric shortcuts perl offers. I typically do something like that with a one-liner, probably something like: `perl -i.old -lne 'if (/^[\s#]Port/i) { print "Port 1234" unless $done +++ } else { print }' sshd_config` [download] Although given that the order of directives in the sshd_config file doesn't matter, that could just be `perl -i.old -lne 'print unless /^[\s#]Port/i; END{print "Port 1234"}' + sshd_config` [download] I don't see a whole lot of point in prompting for the port number, but if you want it separated out of the script you could use `PORT=1234 perl -i.old -lne 'print unless /^[\s#]Port/i; END { print " +Port $ENV{PORT}" }' sshd_config` [download] or `perl -i.old -lne 'BEGIN { $sshport = pop }; print unless /^[\s#]Port/ +i; END { print "Port $sshport" }' sshd_config 1234` [download] All of these are untested, sorry.	[reply] [d/l] [select]
Re^2: Defactor this code by smahesh (Pilgrim) on Feb 11, 2007 at 07:34 UTC
Hi sfink, Your solution is succint and will most likely work as the OP intended. But, I would recommend the OP to use his/her version updated with the comments from other monks. Since the OP is new to perl, a more verbose solution instead of a succint solution may be better in terms of simplicity, understandability and maintainability. Having said that, I would also strongly recommend the OP to try to understand the perl one-liner version - as a good learning exercise. Regards, Mahesh	[reply]


Clear questions and runnable code get the best and fastest answer
	PerlMonks