in reply to improov mein k0den

Your Perl code looks functional, so aside from using other packages, I would only critique the style and some minor points.
  1. http://www.cc2e.com/Default.aspx 'Code Complete' is an excellent style guide for any language
  2. http://www.oreilly.com/catalog/perlbp/ 'Perl Best Practices' is another book specifically for Perl
#!/usr/bin/env perl # I'm just assuming the above line works...? # This program presents a list of hard-coded hostnames to the user and + lets them ssh to one by choosing it's number # ^^ give love to da maintenance programmer use strict; # Good!! use warnings; # Better!! # It's good practice to identify your globals by making them upper-cas +e our $USER = 'brannont'; our @HOSTS = qw( secdevfdsapp01 dopey bonnie ) ; # Use full path of program for exec to avoid security issues our $SSH_PROGRAM = '/usr/bin/ssh'; # Moved the main program execution up here so the maintenance programm +er doesn't have to read the whole program to see what it does... our $SSH_ARGS = &get_ssh_args(); print $SSH_ARGS, "\n" ; exec $SSH_PROGRAM, $SSH_ARGS; # Gets the selected hostname from the user and returns # the arguments we'll need to pass to the 'ssh' command sub get_ssh_args { my $chosen_hostname = &choose_host(); return &ssh_string($chosen_hostname) ; } # For a given host, return the ssh arguments to allow us to # login as a hard-coded user on that host sub ssh_string { my $host = shift; # Suggest also appending your domain here: return &username() . "@" . $host . &domain(); # 'Programming Perl' suggests avoiding use of printf() # sprintf "%s@%s", $user, $host } # Prints the list of hosts with numbers in front to allow # user to make a selection sub printchoices { print "\n\n", "ssh hosts\n", "---------\n"; for (my $i=0; $i < &num_hosts(); ++$i) { print "[$i] ", &hostname_by_num($i), "\n" ; } } # prints a prompt for user input sub prompt { return "\n\nssh to: " ; } # Gets the user's chosen host sub getchoice { print &prompt(); my $choice = <STDIN> ; chomp $choice; return $choice; } # Get the user's chosen host (by number) or die trying # renamed from 'mainloop' sub choose_host { my $chosen_host; while ( !$chosen_host ) { &printchoices(); my $user_choice = &getchoice(); if( ($user_choice =~ /^\d+$/) && ($user_choice >= 0) && ($user_choice < &num_hosts()) ) { $chosen_host = $user_choice; } } # May as well dereference the name here... return &hostname_by_num($chosen_host); } # These three functions are simple lookups to the globals # now, but in the future they can be expanded sub num_hosts { return scalar(@HOSTS); } sub hostname_by_num { return $HOSTS[ $_[0] ]; } sub username { return $USER; } # In the future, this could return our domain # eg. ".example.com", possibly from a global sub domain { return ""; }
The program could be made a lot simpler by combining functions, etc..., but thats not necessarily an advantage from the long term maintenance perspective.

On the other hand, the program was so simple that it's not going to be much of a headache to maintain in the future as-is. The changes I suggested are what I'd suggest to a novice Perl programmer who turned in the above code for deployment on a production system.

Replies are listed 'Best First'.
Re^2: improov mein k0den
by TGI (Parson) on Mar 29, 2007 at 19:48 UTC
    Why not use a perl style foreach loop:
    foreach my $host_num ( 0..num_hosts()-1 ) { print "[$host_num] ", hostname_by_num($host_num), "\n" ; }

    Better yet why not change the lookup function to index from 1 and prevent accidentally mucking about with HOSTS.

    use Scalar::Util qw(looks_like_number); sub hostname_by_num { my $num = shift; # validate our argument defined $num or return undef; $num >= 1 or return undef; looks_like_number( $num ) or return undef; $num--; # convert into array index. return exists $HOSTS[$num] ? $HOSTS[$num] : undef; }

    Now you can simplify choose_host.

    sub choose_host { my $chosen_host; while ( !defined $chosen_host ) { printchoices(); $chosen_host = hostname_by_num( getchoice() ); } return $chosen_host; } # for loop can now be: sub printchoices { print join "\n", "\n", "ssh hosts", "-"x8; foreach my $host_num ( 1..num_hosts() ) { print "[$host_num] ", hostname_by_num($host_num), "\n" ; } }

    The concatenation in ssh_string is hard to read, try join with a null string for assembling the return value:

    sub ssh_string { my $host = shift; return join '', username(), '@', $host, domain(); }

    Also, ditch the unneeded & sigil from your subroutine calls. It can have unexpected consequences. See perlsub.

    Update: The relevent bits from perlsub, with emphasis added:

    ...If a subroutine is called using the & form, the argument list is optional, and if omitted, no @_ array is set up for the subroutine: the @_ array at the time of the call is visible to subroutine instead. This is an efficiency mechanism that new users may wish to avoid.
    ...
    Not only does the & form make the argument list optional, it also disables any prototype checking on arguments you do provide. This is partly for historical reasons, and partly for having a convenient way to cheat if you know what you're doing. See Prototypes below.


    TGI says moo