instance_variable_set in constructor

Diagnosing the problem

What you're doing here is a fairly simple example of metaprogramming, i.e. dynamically generating code based on some input. Metaprogramming often reduces the amount of code you need to write, but makes the code harder to understand.

In this particular case, it also introduces some coupling concerns: the public interface of the class is directly related to the internal state in a way that makes it hard to change one without changing the other.

Refactoring the example

Consider a slightly longer example, where we make use of one of the instance variables:

class Foo
  def initialize(opts={})
    opts.each do |k, v|
      instance_variable_set("@#{k}", v)
    end
  end

  def greet(name)
    greeting = @greeting || "Hello"
    puts "#{greeting}, name"
  end
end

Foo.new(greeting: "Hi").greet

In this case, if someone wanted to rename the @greeting instance variable to something else, they'd possibly have a hard time understanding how to do that. It's clear that @greeting is used by the greet method, but searching the code for @greeting wouldn't help them find where it was first set. Even worse, to change this bit of internal state they'd also have to change any calls to Foo.new, because the approach we've taken ties the internal state to the public interface.

Remove the metaprogramming

Let's look at an alternative, where we just store all of the opts and treat them as state:

class Foo
  def initialize(opts={})
    @opts = opts
  end

  def greet(name)
    greeting = @opts.fetch(:greeting, "Hello")
    puts "#{greeting}, name"
  end
end

Foo.new(greeting: "Hi").greet

By removing the metaprogramming, this clarifies the situation slightly. A new team member who's looking to change this code for the first time is going to have a slightly easier time of things, because they can use editor features (like find-and-replace) to rename the internal ivars, and the relationship between the arguments passed to the initialiser and the internal state is a bit more explicit.

Reduce the coupling

We can go even further, and decouple the internals from the interface:

class Foo
  def initialize(opts={})
    @greeting = opts.fetch(:greeting, "Hello")
  end

  def greet(name)
    puts "#{@greeting}, name"
  end
end

Foo.new(greeting: "Hi").greet

In my opinion, this is the best implementation we've looked at:

1. There's no metaprogramming, which means we can find explicit references to variables being set and used, e.g. with an editor's search features, grep, git log -S, etc.
2. We can change the internals of the class without changing the interface, and vice-versa.
3. By calling opts.fetch in the initialiser, we're making it clear to future readers of our class what the opts argument should look like, without making them read the whole class.

When to use metaprogramming

Metaprogramming can sometimes be useful, but those situations are rare. As a rough guide, I'd be more likely to use metaprogramming in framework or library code which typically needs to be more generic (e.g. the ActiveModel::AttributeAssignment module in Rails), and to avoid it in application code, which is typically more specific to a particular problem or domain.

Even in library code, I'd prefer the clarity of a few lines of repetition.