[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