[PATCH conductor] BZ 790516 - Only accept ipv4, hostname addresses for instances
Martyn Taylor
mtaylor at redhat.com
Wed Feb 15 11:17:11 UTC 2012
On 02/15/2012 01:01 AM, Matt Wagner wrote:
> On Tue, Feb 14, 2012 at 07:29:49PM -0500, Matt Wagner wrote:
>>
>> +# Extract 'ipv4' and 'hostname' addresses from Deltacloud
>> +def extract_addresses(address_list)
>> + addresses = []
>> + address_list.each do |address|
>> + addresses<< address[:address] if ['ipv4', 'hostname'].include?(address[:type])
>> + end
>> + addresses
>> +end
> By the way, slightly outside the scope of a normal patch email, but I'm
> curious -- can anyone see a way to refactor the above method to not use
> the "addresses" array without incurring an extra array iteration? I
> rewrote this a few ways, and this was the least-bad, but I still feel
> like there's something better out there.
>
> I initially wrote it as a one-liner:
>
> address_list.select{|a| ['ipv4', 'hostname'].include?(a[:type])}.
> collect{|a| a[:address]}
>
> (Well, that is "one line" wrapped to 2 in my email client.)
>
> But this was really ugly to read, and it required iterating through two
> arrays instead of one. (Granted, it's not likely to ever be more than a
> handful of elements, but still...)
>
> I then rewrote it as the version that made it into the patch above. I
> tried to take it a step further and eliminate the "addresses" array, so
> that it was just a block:
>
> address_list.collect do |address|
> address[:address] if ['ipv4', 'hostname'].include?(address[:type])
> end
>
> That's the code I really wanted, but it doesn't quite work -- "collect"
> will capture a nil wherever the conditional in the block is false.
> Replacing the last line with "end.compact" feels like a cheap hack, plus
> it ultimately causes another array iteration.
>
> Does anyone have a cleaner solution?
>
> -- Matt
This is a good discussion.
I'm glad you chose the more efficient method and not the 1 liner. Which
most probably would have. I've had a think and a play and I think the
best way to do this is, is what you have in your patch; the only other
way I can think of, is to define your own method or monkey patch the
Array class with something similar to collect but without adding nil
values. But that's far too extreme.
I think there is a trade off here. memory v performance. I think the
memory overhead (of defining another array) far outweighs the
performance of iterating over the same list twice. It's great that you
have really thought about this and not just picked the first thing that
worked.
Good stuff mate
Martyn
More information about the aeolus-devel
mailing list