top | item 7429335

(no title)

feca | 12 years ago

Another problem with the approach from the blog post is locality. If you need to draw 10 numbers in all places, then configuring that value in the module will work. But localy, when you look at the code, it won't tell you how many numbers you are drawing.

If you have this in many different places:

    MegaLotto::Drawing.new.draw
You don't have the information of how many numbers you are getting back. That's not a big deal, but adds to the cognitive load (or requires some comments). Also, if you need to draw different numbers in several different places, you will have to change the configuration many times:

    # First use, we need 10 numbers
    MegaLotto.configure do |config|
      config.drawing_count = 10
    end

    MegaLotto::Drawing.new.draw

    # Second use, we need 6 numbers
    MegaLotto.configure do |config|
      config.drawing_count = 6
    end

    MegaLotto::Drawing.new.draw
And as soon as you do that, you may need to take multi-threading into account, because you are mutating the class.

Extrapolating, it is like defining the size of an array:

    Array.new(4)
If instead you configure Array.new to have a given size for all instantiations, you also lose locality and you may run into thread safety issues.

In the case of MegaLotto::Drawing.new needing multiple configuration options, you can use keyword arguments. If you need too many arguments, maybe the abstraction is wrong. Even if you want to move forward with too many arguments, you can add getters/setters to the newly created instance:

   # Another approach which modifies the instance
   drawing = MegaLotto::Drawing.new
   drawing.size = 10
   drawing.draw #=> returns ten numbers
But this is not optimal design given the elements we have.

discuss

order

danso|12 years ago

Ah yes, mutating the class is most definitely a concern...which is why I give you an additional +1 for advocating for a Hash to be passed into the constructor.

This seems to be what the Twitter gem did too, in its newest versions. AFAIK, the Twitter configuration was a mutation to the class via config object, and now it's been revised to be thread safe:

https://github.com/sferik/twitter#configuration

    client = Twitter::REST::Client.new do |config|
      config.consumer_key        = "YOUR_CONSUMER_KEY"
      config.consumer_secret     = "YOUR_CONSUMER_SECRET"
      config.access_token        = "YOUR_ACCESS_TOKEN"
      config.access_token_secret = "YOUR_ACCESS_SECRET"
    end