I have this piece of Ruby code that sends random gifs. At the moment, this is what my code looks like, but I've heard that using eval on a string is bad practice. What would be some better methods of making this work?
gifs = ["File.open('1.gif')", "File.open('2.gif')", "File.open('3.gif')", "File.open('4.gif')", "File.open('5.gif')"]
eval gifs.sample(10).pop
Any help would be greatly appreciated! Thanks!
One thing you're going to hear about over and over, because it's important is this:
The eval function is always extremely dangerous because what it does is unprecictable. Unlike code you've written and audited, stuff you funnel into eval can contain data you didn't anticipate, especially that which is submitted by users, which is where things can go very, very, very wrong. Of all the bugs to have in your code, any that allows arbitrary code execution is the absolute worst. The best way to avoid these bugs is to avoid using eval in the first place.
The good news is that 999 times out of 1000 there's a better way of doing it, and this is one of those 999 times. The easy fix is to do this the Ruby way:
gifs = %w[
1.gif
2.gif
3.gif
4.gif
5.gif
]
# Select one random image
File.open(gifs.sample)
It's not clear what the intent is with sample(10) which returns an array of ten random entries, and then pop does. That array (of now nine) is thrown out, so there's no point in it being there in the first place. That seems like a lot of code that perhaps was a result of earlier iterations.
Now with Ruby always think about transforming things step-by-step towards your goal, not doing it in huge chunks. Ruby is a very expressive language, so you can actually do this:
gifs = (1..5).map { |n| "#{n}.gif" }
Where that expands an arbitrary range to an array of form [ '1.gif', '2.gif', ... ]. This means you can change one digit and get a lot of work done for "free" since your code will cascade that change right through properly. In your original code you need to copy-paste a bunch of stuff, which is generally bad.
In Ruby we call this "Don't Repeat Yourself" or DRY for short. DRY code is code that's minimal, yet still easy to follow.
The easiest way to DRY things up is to think of what "responsibility" each part of your code has. The gifs array should define which images are available for reading. It should not tell you how to open them, nor do the opening for you. That's for a later bit of code to handle.
By keeping the array just filenames you can later add things like this:
gifs.select! do |path|
File.exist?(path)
end
Were you can quickly screen out files that don't exist before the random selection process.
shuffle and cycle through them instead.