Well, you're clearly a C programmer, since you use printf unnecessarily and you use the C-style for loop. :-)

I'd say you've done a fairly conventional procedural decomposition of the task. You have four or five subroutines, but they're very task specific (that is, they're not resuable) because they either have hard-coded values or they share a dependency on some external data. One good solution in cases like this is to go object oriented.

But that's not the only possible approach. My preference, at least for something this simple, would be to genericize the subroutines to eliminate their hard-coded data and their dependency on external variables, making all that stuff parameters instead. And really, there's no reason for so many subroutines. One will do.

#!perl
use strict;
use warnings;

exec 'ssh', join '@',
  'brannont',
  menu( [qw( secdevfdsapp01 dopey bonnie )],
    "\n\nssh hosts\n---------\n", "\n\nssh to: " );


# returns one of the items in the array passed as the first arg
sub menu
{
  my( $ar, $hdr, $prompt ) = @_;
  local @ARGV;
  {
    print $hdr if defined $hdr;
    print "[$_] $ar->[$_]\n" for 0 .. $#{$ar};
    print $prompt if defined $prompt;
    my $c = <>; chomp $c;
    return $ar->[$c] if $c =~ /^\d+$/ && $c >= 0 && $c <= $#{$ar};
    print "invalid choice\n";
    redo;
  }
}
[download]

I eliminated your printf and C-style for loop. I also fixed a bug: testing of the user input should first check that the string is a number before doing numeric tests on it. Also, by unsetting @ARGV, I ensure that input is from the console. You may or may not want this.

Just kindof a side note, as I'm sure it's common knowledge. The following won't run on modern linuxes.

#!perl

use strict;
print "hiya\n";
[download]

It's considered a security problem to have a non-absolute path in a she-bang.

The usual way around it is #!/usr/bin/env perl.

-Paul

Well . . . (gah, this almost got eaten by the server hiccups)

• Why use sprintf when "$user\@$host" would work just fine (or join( '@', $user, $host ))
• Don't use printf where a simple print will do
• You don't need the C-style for loop: for my $idx ( 0 .. $#host ) { print "[$idx] $host[$idx]\n"; }
• Name your arrays with plurals (e.g. @hosts)
• No error checking on the exec call (that's kinda pedantic though)

IO::Prompt

Nice idea, but...

Watch what happens on Windows*:

C:\>perl 607274.pl
Cannot write to terminal: No such file or directory at 607274.pl line 
+12
[download]

A check of the IO::Prompt man page splains that this means

your environment has no /dev/tty available, in which case there isn't much you can do with this module. Sorry.

Diving into the module's source, we find that it is indeed rife with the following:

open my $OUT, ">/dev/tty" or croak "Cannot write to terminal: $!";
[download]

And yet, inexplicably (ironically?), the documentation's DEPENDENCIES, INCOMPATIBILITIES, and BUGS AND LIMITATIONS sections are coyly silent on this shameless OS bondage.

Somehow this doesn't strike me as "perl best practices". Very disappointing.

* Not just Windows, but any other OS which doesn't represent its terminal in the filesystem via a file named /dev/tty. Which is most of them.

A word spoken in Mind will reach its own level, in the objective world, by its own weight

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 ""; }
[download]

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" ;
}
[download]

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;
}
[download]

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" ;
    }
}
[download]

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();
}
[download]

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