On Monday 24 August 2009 02:24:08 pm Brandon Larocque wrote:
From what I’ve read, recently, people tend to look down on the eval
function? Why is this?
It has a few specific uses – irb wouldn’t work without it, for example
– but
in general, there is a better way to do it, whatever it is.
Personally, my biggest beef with it is that I simply find it less
readable,
because, among other things, my text editor doesn’t highlight Ruby
syntax
inside strings, even if those strings are being fed into eval.
But, let’s take a contrived example. Suppose we didn’t have
attr_accessor:
%w(mine yours henry’s).each do |name|
eval "
def #{name}
@#{name}
end
def #{name}= value
@#{name} = value
end
"
end
Can you spot the problem with that code?
Now, imagine that list of names came from elsewhere, or worse, from user
input. You could easily end up with a SQL injection problem, or
something
worse. There’s just a whole class of weird, hard-to-debug problems that
won’t
happen unless you use eval.
Finally, people dislike eval for the same reason they dislike goto – in
general, if you’re abusing eval, it’s a sign that you should refactor
your
code, and eval will just lead you to a spaghetti-code mess.
I have a few classes that have multiple names
I actually have no idea what you mean by this. Your example code shows
exactly
zero indication of “classes that have multiple names”.
( $jobarea == “hi” or $jobarea == “es” ) ? (loop = 9) : ( ($jobarea ==
“ca”) ? (loop = 8) : (loop = 7) )
expcost = [] ; exppay = [] ; pay = [] ; name = []
for i in (1…loop) do
eval(“expcost.push $#{$jobarea}_job#{i}.expcost”)
eval(“exppay.push $#{$jobarea}_job#{i}.exppay”)
eval(“pay.push $#{$jobarea}_job#{i}.pay”)
eval(“name.push $#{$jobarea}_job#{i}.name”)
end
First of all, you’re using global variables. Giant red flag there, just
like
eval. Is there a reason those aren’t either constants or local
variables?
So, first, I’d restructure it a bit:
Jobs = {
‘hi’ => [Job.new(…), Job.new(…), …],
‘es’ => …
}
You get the idea – put things in arrays or hashes, not global
variables.
Glancing over at kallen’s mail, it seems he came up with the exact same
thing…
Aside from making it easier to loop through, it makes it much easier to
add
other jobs later on. For example, you could do:
Jobs[‘hi’].push Job.new(…)
You don’t have to change the loop or anything like that – the number of
jobs
in that area can be found with:
Jobs[‘hi’].length
Now, those “bunch of loops” you didn’t want to write:
results = Hash.new{|k,v| k[v]=[]}
Jobs.each_value do |jobs_in_area|
jobs_in_area.each do |job|
results[‘expcost’].push job.expcost
results[‘exppay’].push job.exppay
results[‘pay’].push job.pay
results[‘name’].push job.name
end
end
Even that is a bit repetitive. I’d probably turn that inner loop into
something like this:
%w(expcost exppay pay name).each do |field|
results[field].push job.send(field)
end
