ruby wizards, help me beautify skanky code

[Giles Bowkett]

here it is:

@inducers = []
@inhibitors = []
@substrates = []

Interaction.find_all_by_involvement_type("Inducer").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Inhibitor").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Substrate").each do |interaction|
@substrates.push(Drug.find(interaction.drug_id).name)
end

@inducers.uniq!
@inhibitors.uniq!
@substrates.uniq!

obviously there's quite a bit of duplication in this. I am utterly
certain that with currying or something similar, this code could be
much, much briefer, much more DRY.

the "uniq!" calls are nonoptimal, but I didn't see any obvious way to
exclude duplicates in the process of building the arrays without using
multiple ActiveRecord find() calls, and since those turn into SQL
queries, it seemed wasteful.

the view file for this process is equally lame:

<h2>Inducers</h2>
<ul><% for inducer in @inducers %>
<li><%= inducer %></li>
<% end %></ul>

<h2>Inhibitors</h2>
<ul><% for inhibitor in @inhibitors %>
<li><%= inhibitor %></li>
<% end %></ul>

<h2>Substrates</h2>
<ul><% for substrate in @substrates %>
<li><%= substrate %></li>
<% end %></ul>

again, loads of needless repetition. I'm utterly certain that both
these code samples could be much, much more elegant, but other than a
few vague ideas about currying I'm really at a loss to say exactly
how. an inelegant view I can cope with, but the sheer mind-numbing
repetition of the controller code is utterly mortifying, more so when
I know with absolute certainty that Ruby has some way of streamlining
that process immensely, I just don't know specifically what it
actually is.

I could just turn it into a private method, I suppose, something like

@inducers = uniq_list("Inducer")
@inhibitors = uniq_list("Inhibitor")
@substrates = uniq_list("Substrate")

private
def uniq_list(search_term)
items = []
Interaction.find_all_by_involvement_type(search_term).each do |interaction|
items.push(Drug.find(interaction.drug_id).name)
end
items.uniq! || []
end

in fact that's probably what I'll do -- but what with the whole
metaprogramming thing, I'd like to come up with something cleaner, one
of those Lispy things. any ideas would be massively appreciated. I
suppose actually in the course of writing this e-mail I've figured out
the answer to the question myself, but I'm pretty sure there's a next
step beyond this which would make the code more elegant.

 

[Jan Svitok]

here it is:

@inducers = []
@inhibitors = []
@substrates = []

Interaction.find_all_by_involvement_type("Inducer").each do |interaction|
@inhibitors.push() typo!!! @inducers
end

you can write this as

@inducers +=
Interaction.find_all_by_involvement_type("Inducer").map do
|interaction|
Drug.find(interaction.drug_id).name
end

If you don't mind the order, you can get rid of uniq! by using Set.new
instead of [].

Interaction.find_all_by_involvement_type("Inhibitor").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Substrate").each do |interaction|
@substrates.push(Drug.find(interaction.drug_id).name)
end

@inducers.uniq!
@inhibitors.uniq!
@substrates.uniq!

obviously there's quite a bit of duplication in this. I am utterly
certain that with currying or something similar, this code could be
much, much briefer, much more DRY.

the "uniq!" calls are nonoptimal, but I didn't see any obvious way to
exclude duplicates in the process of building the arrays without using
multiple ActiveRecord find() calls, and since those turn into SQL
queries, it seemed wasteful.

the view file for this process is equally lame:

<h2>Inducers</h2>
<ul><% for inducer in @inducers %>
<li><%= inducer %></li>
<% end %></ul>

<h2>Inhibitors</h2>
<ul><% for inhibitor in @inhibitors %>
<li><%= inhibitor %></li>
<% end %></ul>

<h2>Substrates</h2>
<ul><% for substrate in @substrates %>
<li><%= substrate %></li>
<% end %></ul>

You can make this a method in the helper, or use sub-template.

again, loads of needless repetition. I'm utterly certain that both
these code samples could be much, much more elegant, but other than a
few vague ideas about currying I'm really at a loss to say exactly
how. an inelegant view I can cope with, but the sheer mind-numbing
repetition of the controller code is utterly mortifying, more so when
I know with absolute certainty that Ruby has some way of streamlining
that process immensely, I just don't know specifically what it
actually is.

I could just turn it into a private method, I suppose, something like

@inducers = uniq_list("Inducer")
@inhibitors = uniq_list("Inhibitor")
@substrates = uniq_list("Substrate")

private
def uniq_list(search_term)
items = []
Interaction.find_all_by_involvement_type(search_term).each do |interaction|
items.push(Drug.find(interaction.drug_id).name)
end
items.uniq! || []
end

same here:
def uniq_list(search_term)
Interaction.find_all_by_involvement_type(search_term).map do |interaction|
Drug.find(interaction.drug_id).name
end.uniq! || []
end

 

[Zed A. Shaw]

here it is:

@inducers = []
@inhibitors = []
@substrates = []

Interaction.find_all_by_involvement_type("Inducer").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Inhibitor").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Substrate").each do |interaction|
@substrates.push(Drug.find(interaction.drug_id).name)
end

@inducers.uniq!
@inhibitors.uniq!
@substrates.uniq!

require 'set'

inv_types = ["Inducer", "Inhibitor", "Substrate"]
@results = {}

inv_types.each do |inv|
m = Interaction.find_all_by_involvement_type(inv).map { |inter|
Drug.find(inter.drug_id).name)
}
@results[inv] = Set.new(m)
end

But I didn't run this, don't know how Drug objects interact with Set, and I'm sure it could be made more readable or faster (but not both). Also you could do a SQL statement or fancier find that'll make this much more efficient.

Advantage of this is when you get new involvement types you just update the inv list, and you could probably even do away with that and place those in a table instead. Your views can also change to just iterate over the contents of @results and become general as well.

 

[dblack]

Hi --

here it is:

@inducers = []
@inhibitors = []
@substrates = []

Interaction.find_all_by_involvement_type("Inducer").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)

I assume you mean @interactions.push here.

end
Interaction.find_all_by_involvement_type("Inhibitor").each do
|interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)
end
Interaction.find_all_by_involvement_type("Substrate").each do
|interaction|
@substrates.push(Drug.find(interaction.drug_id).name)
end

@inducers.uniq!
@inhibitors.uniq!
@substrates.uniq!

obviously there's quite a bit of duplication in this. I am utterly
certain that with currying or something similar, this code could be
much, much briefer, much more DRY.

the "uniq!" calls are nonoptimal, but I didn't see any obvious way to
exclude duplicates in the process of building the arrays without using
multiple ActiveRecord find() calls, and since those turn into SQL
queries, it seemed wasteful.

Here's an untested version that might be improved upon but nonetheless
might give you some ideas:

%w{ inducer inhibitor substrate }.each do |thing|
condition =
instance_variable_set("@#{thing}s",
Interaction.findall,
:conditions => "involvement_type = #{thing.upcase},
:include => "drug").map {|i| i.drug.name}.uniq
end

I've taken the liberty of changing Drug.find(interaction.drug_id).name
to i.drug, on the theory that if interactions have a drug_id field,
then they belong_to :drug, and therefore should have a drug method.


David

--
David A. Black | (e-mail address removed)
Author of "Ruby for Rails" [1] | Ruby/Rails training & consultancy [3]
DABlog (DAB's Weblog) [2] | Co-director, Ruby Central, Inc. [4]
[1] http://www.manning.com/black | [3] http://www.rubypowerandlight.com
[2] http://dablog.rubypal.com | [4] http://www.rubycentral.org

 

[dblack]

Hi --

here it is:

@inducers = []
@inhibitors = []
@substrates = []

Interaction.find_all_by_involvement_type("Inducer").each do |interaction|
@inhibitors.push(Drug.find(interaction.drug_id).name)

I assume you mean @interactions.push here.

I mean I assume you mean @inducers.push here.


David

--
David A. Black | (e-mail address removed)
Author of "Ruby for Rails" [1] | Ruby/Rails training & consultancy [3]
DABlog (DAB's Weblog) [2] | Co-director, Ruby Central, Inc. [4]
[1] http://www.manning.com/black | [3] http://www.rubypowerandlight.com
[2] http://dablog.rubypal.com | [4] http://www.rubycentral.org

 

[Zed A. Shaw]

I'm assuming that you're using rails here. Maybe not... I'm trying to get
everything with one query on the db but I haven't tested it so it may not
work very well.

@interactions = Interaction.findall,
:conditions => ["involvement_type
in (?)", %w(Inducer Inhibitor Substrate)],
:include =>
:drugs).group_by(&:involvement_type)

Yes! That's what I was talking about. Couldn't think the voodoo up though.

 

[Giles Bowkett]

Here's an untested version that might be improved upon but nonetheless

might give you some ideas:

%w{ inducer inhibitor substrate }.each do |thing|
condition =
instance_variable_set("@#{thing}s",
Interaction.findall,
:conditions => "involvement_type = #{thing.upcase},
:include => "drug").map {|i| i.drug.name}.uniq
end

I've taken the liberty of changing Drug.find(interaction.drug_id).name
to i.drug, on the theory that if interactions have a drug_id field,
then they belong_to :drug, and therefore should have a drug method.

change upcase to capitalize (and pull out the line condition= ) and I
think this'll be a massive improvement. I do like the solutions that
made it all one SQL call, though.

this one builds on what you've done here and merges it as one SQL query:

@interactions = Interaction.findall,
:conditions =>
["involvement_type in (?)", %w(Inducer Inhibitor Substrate)],
:include =>
:drugs).group_by(&:involvement_type)

@interactions.keys.each do |key|
instance_variable_set( "@#{key}", interactions[key].map {
|i|i.drug.name}.to_set )
end

just for perspective, though, these three involvement types are the
only possible involvement types. so merging both solutions you could
probably do something like:

Interaction.find_all.group_by(involvement_type).keys.each do |key|
instance_variable_set( "@#{key.downcase.pluralize}",
interactions[key].map {
|i|i.drug.name}.to_set )
end

that is in fact staggeringly more elegant than the monstrosity I
started this off with. the only hesitation I have there is that it
could in fact be too compact for subsequent programmers to comfortably
maintain. (the other thing is I'm on a client that doesn't use a
fixed-width font so I can't be sure of the indentation, although
that's hardly mission-critical.)

still, 15 lines of code down to 4 is pretty good! if you look at the
time on the original message you'll realize I am in fact in need of
sleep, but I should be able to pop this into the app pretty soon to
see what happens.

 

[dblack]

Hi --

change upcase to capitalize (and pull out the line condition= ) and I

Whoops -- a cut-and-paste accident

[...]

just for perspective, though, these three involvement types are the
only possible involvement types. so merging both solutions you could
probably do something like:

Interaction.find_all.group_by(involvement_type).keys.each do |key|
instance_variable_set( "@#{key.downcase.pluralize}",
interactions[key].map {
|i|i.drug.name}.to_set )
end

You can also do away with isolating the keys and then using them to
dig out the values -- just give yourself both:

Interaction.findall).group_by(&:involvement_type).each do |key,value|
instance_variable_set("@#{key.downcase.pluralize}",
value.map {|i| i.drug.name }.to_set)
end

(I'm not sold on to_set instead of uniq, but either should be OK.)


David

--
David A. Black | (e-mail address removed)
Author of "Ruby for Rails" [1] | Ruby/Rails training & consultancy [3]
DABlog (DAB's Weblog) [2] | Co-director, Ruby Central, Inc. [4]
[1] http://www.manning.com/black | [3] http://www.rubypowerandlight.com
[2] http://dablog.rubypal.com | [4] http://www.rubycentral.org

 

[MonkeeSage]

obviously there's quite a bit of duplication in this. I am utterly
certain that with currying or something similar, this code could be
much, much briefer, much more DRY.

I wrote an ugly / simple / buggy implementation of currying for ruby
here:
http://rightfootin.blogspot.com/2006/09/spicey-curry-in-your-ruby.html

Regards,
Jordan

 

[Ezra Zygmuntowicz]

@interactions = Interaction.findall,
:conditions =>
["involvement_type in (?)", %w(Inducer Inhibitor Substrate)],
:include =>
:drugs).group_by(&:involvement_type)

@interactions.keys.each do |key|
instance_variable_set( "@#{key}", interactions[key].map {
|i|i.drug.name}.to_set )
end

script/plugin install svn://rubyforge.org/var/svn/ez-where

@interactions = Interaction.find_whereall, :include => :drugs) { |
interact, drug|
interact .involvement_type === %w(Inducer Inhibitor Substrate)
}.group_by(&:involvement_type)


@interactions.keys.each do |key|
instance_variable_set( "@#{key}", interactions[key].map {
|i|i.drug.name}.to_set )
end



-Ezra

 

[Giles Bowkett]

OK -- here's what I have at the moment.

controller:

def drugs
Interaction.findall, :include =>
"drug").group_by(&:involvement_type).each do |key, value|
instance_variable_set( "@#{key.downcase.pluralize}",
value.map { |i|i.drug.name}.uniq.sort )
unless value.nil?
end
renderpartial => "drugs")
end

view:

<% for thing in %w{ Inducer Inhibitor Substrate } %>
<% things = eval("@#{thing.downcase.pluralize}") %>
<% unless things.nil? %>
<h2><%= thing %>s</h2>
<ul><% for name in things %>
<li><%= name %></li>
<% end %></ul>
<% end %>
<% end %>

I chose uniq over to_set because I like the way word ends with the
letter q. (not entirely scientific, I must admit.) I tested both just
to see if they would in fact return the same result and in fact they
returned the same stuff but in a different order, so I added sort to
alphabetize the array contents. the unless value.nil? proved necessary
since some values were in fact returning nil. that could be a bug in
the code, or it could be something I overlooked about the data. not
sure.

the view doesn't look half as pretty as the controller but I think it
also benefits from its brevity, although to be honest I'm not entirely
certain of that. the original version was clunky but obvious; the new
version won't present any problem to a maintenance programmer, as long
as the maintenance programmer is me, and has had some caffeine. the
other guy most likely to work on this code besides myself is a
marketing guy who figured out just enough Rails to be dangerous; there
is a risk in handing him templates which contain eval() statements. I
used the things= assignment at the top to lessen the potential pain;
sometimes adding unnecessary steps is actually a kindness to whoever
maintains the code.

it also has the benefit of reducing the number of eval()s from two to
one. I'm hesitant about eval() statements in general, it seems like
the type of programming which evokes the harshest cries of "Perlish!!"
from the Python community. I think the Python community needs to
lighten up a bit, but there's no denying it looks a bit more hacky
than the original did, even tho the original was unimaginative. very
much undecided about eval(). that's pretty much how references work in
Perl, but Perl references are notorious for spreading confusion
everywhere they go. on the other hand, they're also wonderfully
flexible.

anyway, cheers for the help!

 

[Ross Bamford]

I wrote an ugly / simple / buggy implementation of currying for ruby
here:
http://rightfootin.blogspot.com/2006/09/spicey-curry-in-your-ruby.html

Just for completeness (IMHO currying isn't the right approach for this):

# gem install rubymurray - see http://rubymurray.rubyforge.org
require 'curry'

def find(type)
Interaction.find_all_by_involvement_type(type).map do |interaction|
Drug.find(interaction.drug_id).name
end
end

find_inducers = methodfind).curry("Inducer")
find_inhibitors = methodfind).curry("Inhibitor")
find_substrates = methodfind).curry("Substrate")

# whatever...
p find_inducers[], find_inhibitors[], find_substrates[]