current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have the following function. It can be passed a candidate_string such as the following examples:

  1. "device1 device2"
  2. "device1"
  3. "device.*"
  4. "device.region"
  5. "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:

  1. ['device1', 'device2']
  2. ['device1']
  3. ['device1', 'device2', 'device3', 'device4']
  4. ['device.region']
  5. ['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;
}
share|improve this question
    
Second foreach can be replaced with hash look up. –  mpapec Sep 3 '14 at 21:17
    
@mpapec, can you please explain how? –  Mark Sep 3 '14 at 21:42

1 Answer 1

up vote 1 down vote

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:

  1. List of patterns
  2. 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;
}
share|improve this answer
    
What do you think @results{@found} = 1 does? –  Borodin Sep 13 '14 at 22:13
    
You don't need the values, so @results{@found} = () will do. It sets all the values to undef –  Borodin Sep 13 '14 at 22:20
    
Ok so that was silly, maybe I shouldn't edit my posts after midnight :s Fixed now. Thanks again @Borodin! –  janos Sep 14 '14 at 5:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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