7
\$\begingroup\$

This code will be passed a string which will contain one or more hostnames.

A hostname can be

  • dotted-decimal
  • plain name (like my_host)
  • FQDN or even partially qualified
  • A string containing regex (my_hos[tes])

target_list is an array of known hosts to match any regex against.

Any suggestions on cleaning this up?

sub expand_names {
    my $host = shift;

    my @raw_hosts = split(' ', $host);
    my @hosts;

    my %host_hash;
    foreach my $raw_name (@raw_hosts) {
        $host_hash{$raw_name} = {};
        #if the name might contain regex, let's see what it matched
        if ($raw_name !~ m/^[a-zA-Z0-9-]+$/) {
            foreach my $target (@target_list) {
                if ($target =~ m/^$raw_name$/) {
                    $host_hash{$raw_name}->{$target} = 1;
                }
            }
            #if we didn't match anything, then just use the name
            if (not keys %{$host_hash{$raw_name}} ) {
                $host_hash{$raw_name}->{$raw_name} = 1;
            }
        }
        else {
            # Regular host name or dotted-decimal
            $host_hash{$raw_name}->{$raw_name} = 1;
        }
    }
    # Gather all the hosts we found
    foreach $raw_name  (keys %host_hash) {
        @hosts = (@hosts, (keys %{$host_hash{$raw_name}}));
    }
    return sort @hosts;
}
\$\endgroup\$
6
\$\begingroup\$

Your function expects a single argument that should contain hostnames or patterns separated by spaces:

sub expand_names {
    my $host = shift;
    my @raw_hosts = split(' ', $host);
    foreach my $raw_name (@raw_hosts) {

Why not use multiple arguments?

sub expand_names {
    foreach my $raw_name (@_) {

Since @hosts is empty when you reach this code:

# Gather all the hosts we found
foreach $raw_name  (keys %host_hash) {
    @hosts = (@hosts, (keys %{$host_hash{$raw_name}}));
}

You could simplify:

foreach $raw_name  (keys %host_hash) {
    push(@hosts, keys %{$host_hash{$raw_name}});
}

And actually, I don't see the reason for building a map of maps, when a simple flat map will do. Consider this alternative implementation:

sub expand_names {
    my %hosts;
    foreach my $raw_name (@_) {
        #if the name might contain regex, let's see what it matched
        if ($raw_name !~ m/^[a-zA-Z0-9-]+$/) {
            my $found = 0;
            foreach my $target (@target_list) {
                if ($target =~ m/^$raw_name$/) {
                    $found = $hosts{$target} = 1;
                }
            }
            $hosts{$raw_name} = 1 if not $found;
        } else {
            # Regular host name or dotted-decimal
            $hosts{$raw_name} = 1;
        }
    }
    return sort keys %hosts;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.