1
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ 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? \$\endgroup\$ – vgoff Jul 18 '13 at 2:49
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – tokland Jul 18 '13 at 13:06
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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(). \$\endgroup\$ – Victor Moroz Jul 19 '13 at 16:02
  • \$\begingroup\$ 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/… \$\endgroup\$ – tokland Jul 19 '13 at 16:07
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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