Getting parent relationship

I am trying to find the best way to structure this. I want to avoid if-else statements and reduce the number of code lines.

def get_parent(relationships,GU)
parent = []
if relationships.class == Array
relationships.each do |y|
if y[:relationship_roles][:relationship_role][:to_role][:name] == type
parent << y[:party][:party_id][:id]
end
end
else
if relationships[:relationship_roles][:relationship_role][:to_role][:name] == type
parent << relationships[:party][:party_id][:id]
end
end
return parent
end


Input:

:relationship=>{:name=>nil, :party=>{:party_id=>{:id=>"12344",
:id_type=>"abc"}, :name=>"XYZ", :@active=>"true", :@type=>"Organization"},
:relationship_roles=>{:relationship_role=>{:to_role=>{:name=>"GU"}}}}


The input could be only one relationship or multiple ones, which is why I have the if/else loop.

• You say you have a loop, but you have no loop. You must mean something else. If you had a loop, you would have loop..do..end. What are you trying to get rid of? Nested if statements? – vgoff Jul 18 '13 at 2:49
• No doubt the question is poorly worded, but closing it for this reason... The code is clearly messy, it's not that important how the OP tries to explain it. – tokland Jul 18 '13 at 13:06

Some notes:

• It would be debatable whether it's good practice to have arguments which may contain either single elements or collections.
• If you convert whatever you get to an array (using Array), there is no need to repeat code.
• Don't use each + << (imperative), use map + compact (functional).

I'd write:

def get_parent(relationship_item_or_collection)
Array(relationship_item_or_collection).map do |relationship|
if relationship[:relationship_roles][:relationship_role][:to_role][:name] == type
relationship[:party][:party_id][:id]
end
end.compact
end

• I would say using map/compact here is questionable, inject could be a better alternative if it was not so slow (in R1.8 at least, comparing to each). Totally agree with Array(). – Victor Moroz Jul 19 '13 at 16:02
• Victor, I agree that map+compact is space-inefficient and slower. However, changing the right abstraction for the job with an inject or each (which are lower in the abstraction layer, speciall each) would be a pity, wouldn't it? For me the real solution would be to have an efficient abstraction (a pity it's not in the core). So in my projects I always have compact_map available, either my own implementation or from Facets: github.com/rubyworks/facets/blob/master/lib/core/facets/… – tokland Jul 19 '13 at 16:07

disclaimer: my ruby knowledge is practically nonexistent so the code below might be completely rubbish ruby-wise.

I see two code smells in this code: duplicate code and switch on type.

To eliminate the duplicate code we can apply the extract method refactoring:

def get_parent_for_relationship(relationship, parent)
if relationship[:relationship_roles][:relationship_role][:to_role][:name] == type
parent << relationship[:party][:party_id][:id]
end
end

def get_parent(relationships)
parent = []
if relationships.class == Array
relationships.each do |y|
get_parent_for_relationship(y, parent)
end
else
get_parent_for_relationship(relationship, parent)  end
return parent
end


To make the switch on type even more visible, we can extract another method:

def get_parent_for_relationship(relationship, parent)
if relationship[:relationship_roles][:relationship_role][:to_role][:name] == type
parent << relationship[:party][:party_id][:id]
end
end

def get_parent_for_array_of_relationships(relationships, parent)
relationships.each do |y|
get_parent_for_relationship(y, parent)
end
end

def get_parent(relationships)
parent = []
if relationships.class == Array
get_parent_for_array_of_relationships(relationships, parent)
else
get_parent_for_relationship(relationship, parent)  end
return parent
end


Where you go from here is largely dependent on the calling code. An extensive discussion on switch on type, including strategies on dealing with it if it is really a problem can be found here