Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change

Defactor this code

by xerophyte (Initiate)
on Feb 10, 2007 at 13:01 UTC ( #599350=perlquestion: print w/replies, xml ) Need Help??
xerophyte has asked for the wisdom of the Perl Monks concerning the following question:

Dear Monks, Could you please improve this code, i am new to perl, trying to see if i can do this code in a better way
my $sshport; my $errorstate = 0; print "\n Please Enter Custom port for SSH server:\n"; chomp($sshport = <STDIN>); while (!$errorstate) { if ($sshport =~ /^[1-9]\d{1,9}$/ ) { $errorstate = 1; } else { print " Please Enter valid ssh port number:\n"; chomp($sshport = <STDIN>); } } open (SSHD_CONFIG,'sshd_config') or die ("Could not able to open the f +ile $!"); open (SSHD_CONFIG_NEW,">",'') or die ("Could not create + the new file $!"); while (<SSHD_CONFIG>){ if (/^port/i) { s/Port.+/Port $sshport/; } elsif (/^#port\s/i) { s/#Port.+/Port $sshport/; } print SSHD_CONFIG_NEW $_; } rename('sshd_config','sshd_config.old'); rename('','sshd_config');

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> ); }

    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 $_; }

    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') or die "Can't rename $!";
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 } }
    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
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,">","$") or die ("can't open $ for writing: $!"); while (<SSHD_CONFIG>){ s/^(#?)port.+/$1Port $sshport/i; print SSHD_CONFIG_NEW; } ( rename $filename, "$filename.old" and rename "$", $filename ) or die "rename failed: $!"
    (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.
      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 ::
      After we modify the file with above code it becomes like this
      Port 4444
      Port 4444
      it should be lie
      Port 4444
      Thanks again
        i have one problem the sshd_config sometime looks like this
        #Port 22 Port 4444
        After we modify the file with above code it becomes like this
        Port 4444 Port 4444

        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; }
        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; }
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.


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
    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
    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
    perl -i.old -lne 'BEGIN { $sshport = pop }; print unless /^[\s#]*Port/ +i; END { print "Port $sshport" }' sshd_config 1234
    All of these are untested, sorry.
      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.


Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://599350]
Approved by kyle
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others imbibing at the Monastery: (7)
As of 2017-02-22 04:35 GMT
Find Nodes?
    Voting Booth?
    Before electricity was invented, what was the Electric Eel called?

    Results (324 votes). Check out past polls.