Split a string of hostnames, expand from regex, return non-duplicated names in original order I have the following function. It can be passed a candidate_string such as the following examples:


"device1 device2"
"device1"
"device.*"
"device.region"
"device[123]


Assume that target_list contains at least the following:

['device1', 'device2', 'device3', 'device4']

I want the output of calling the function to return the following:


['device1', 'device2']
['device1']
['device1', 'device2', 'device3', 'device4']
['device.region']
['device1', 'device2', 'device3']


This code works (and I have unit tests for all of the above cases and more), I would like to know if it can be improved on.

I am passing an optional target_list into the function, only in my tests, this is so I don't have to mock that function out.

######################################################################
# sub hostname_regex_expand($host_string, @target_list)
#
# Returns a list of hostnames from $host_string.
#
# $host_string should be a space separated list of 1 or more
# host_names, or regex strings representing host_names. If the
# given host_name contains chars other than [a-zA-Z0-9-], then we
# will try to match the assumed regex against the list of names returned
# by get_hostlist. It is also possible that this is a device name
# of the form device.region or FQDN. In that case,
# we will not match anything from get_hostlist so will use the name
# as provided. If the hostname is just a hostname, then we use it as is.
######################################################################
sub hostname_regex_expand {
    my ($host_string, @target_list) = @_;

    # split by whitespace
    my @host_names = split(' ', $host_string);

    # load hostlist
    if (not @target_list) {
        @target_list = get_hostlist();
    }

    my (%host_hash, @hosts);
    foreach my $host_name (@host_names) {
        my $found = 0;
        foreach my $target (@target_list) {
            if ($target =~ m/^$host_name$/) {
                $found = 1;
                if (not $host_hash{$target}) {
                    $host_hash{$target} = 1;
                    push(@hosts, $target);
                }
            }
        }
        # If we didn't match anything in the target_list, just add this
        # host_name to the hash.
        if (not $found and not $host_hash{$host_name}) {
            push(@hosts, $host_name);
            $host_hash{$host_name} = 1;
        }
    }

    return @hosts;
}
To jump to the conclusion first, I would rewrite the function like this:

sub hostname_regex_expand {
    my ($patterns_ref, $hostnames_ref) = @_;
    my @patterns = @$patterns_ref;
    my @hostnames = $hostnames_ref ? @$hostnames_ref : get_hostlist();

    my %results;
    foreach my $pattern (@patterns) {
        my @found = grep(/$pattern/, @hostnames);
        @found = $pattern unless @found;
        @results{@found} = ();
    }

    return sort keys %results;
}


First of all, the original variable names were not so good: @target_list, %host_hash, and $host_string contain in the name their types (list, hash, string), which is not normal or necessary. What's worse is that @host_names and $host_name are storing host name patterns, not really host names.

In terms of the logic, instead of iterating over each hostname to try to see if it matches a pattern, it would be more straightforward to use a grep.

It seems you really want to use two list parameters:


List of patterns
List of hostnames


So I changed the function to take two list references, it's cleaner than the original.

I took a shortcut that you might not like: instead of adding matched hosts and patterns to @hosts, I use the %results map, and return the sorted keys. The title of your post (and the original) suggests that you want to preserve the original order. If that's a requirement, not a problem, we can refactor like this:

sub hostname_regex_expand {
    my ($patterns_ref, $hostnames_ref) = @_;
    my @patterns = @$patterns_ref;
    my @hostnames = $hostnames_ref ? @$hostnames_ref : get_hostlist();

    my @ordered;
    my %results;
    foreach my $pattern (@patterns) {
        my @found = grep(/$pattern/, @hostnames);
        @found = $pattern unless @found;
        foreach my $item (@found) {
            if (!$results{$item}) {
                push(@ordered, $item);
                $results{$item} = 1;
            }
        }
    }

    return @ordered;
}