4
\$\begingroup\$

I have a small gem that I've been maintaining. This gem provides a simple way to do translation using an alternative Google API.

I've been working with Java last year and recently did some refactoring on that gem, but I don't think it's Rubyist enough.

require_relative 'extractor'
require_relative 'requester'
require_relative 'translator'

module FreeGoogle
  ##
  # Responsible for create instance of {FreeGoogle::Translator}
  # It holds all dependencies for this creation
  class Factory
    def self.create
      extractor = FreeGoogle::Extractor.new
      http_requester = FreeGoogle::Requester.new
      FreeGoogle::Translator.new(extractor, http_requester)
    end
  end
end

Source code

Are factories commonly used in Ruby?

\$\endgroup\$
1
\$\begingroup\$

In short: keep it simple.

I'm not sure if I'm supposed to review code that was not pasted here, but I took a look, since you linked it, and your sample is very brief. I really think that you are over-designing it (and even than your design has flaws):

  • I see no reason for a factory here. Why do you want to use it (we do use them in Ruby, although we often have simpler choice that don't work that well in Java)?
  • Extractor, Translator and Requester hold no state, why would they be objects at all (and not modules) and require instantiating? Factory can't even be instantiated, so (assuming you keep it) it definitely has no reason to be a class.
  • Extractor, Requester and Factory all have single method each. Translator also has initialize, but #translate basically only calls methods of Extractor and Requester. Don't take single responsibility literally, request and extraction could reasonably be methods in the same module (or in a class, this could be justified even with no instantiating here, because Ruby doesn't really allow private module methods and #extract and #request rather should be private) which responsibility is 'translation'. Lazy class is a code smell, 4 lazy classes 4 times more so.
  • Assuming you keep your responsibilities that much divided, forming a request URL is definitely responsibility of Requester (one more reason to get rid of some classes, as Translator would basically do nothing than).

The trick with patterns is not to apply them blindly. You wanted to gain better readability, testability and decoupling - did you?

  • Your coude certainly is hard to read, you literally need to switch files every second line to understand it
  • You didn't realy make it any more testable, those methods would be perfectly testable if they were put together. Your gem would need to get much, much more complicated before having this 4 classes would provide a gain in testability. There is no point to care about it now, it is more than likely that would your gem grow, you would need to refactor it anyways.
  • You didn't really decouple anything - factory decouples instantiation, but there shouldn't even be any, as your objects hold no state. Request and extraction are coupled anyways, because if you changed your request(as in: used different translation engine) your extraction logic likely would need to change as well.

Code structure that I'd advise: one mixin to extract common logic from translators, and modules with engine-specific logic (that is request's and extraction), you have only google now, but it seems reasonable to assume more might be added in future.

module Translator
  def translate(text)
    # something like:
    data = request text
    extract data
  end
end

module FreeGoogle
  extend Translator

  def self.request
    # ...
  end
  def self.extract
    # ...
  end
  # you might, or might not add ::url helper method, but this is
  #   somewhat complex so you should I guess, if only for testing it
  def self.url
    # ...
  end
end

Of course you should separate them in different files and use other module as a namespace, like all other gems do. This is simple, but flexible, maintanable and idiomatic("Ruby-ish"). To test Translator mixin properly you would need a simple mock, but that's no biggie. Classes could be considered to make some methods private, but again, this is not that important, if someone calls FreeGoogle::url nothing bad can happen, and Ruby has easy ways to call private methods anyways.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much @Borsunho to spent your time to review the entire gem! I got your advice. As you said, sometimes I take the Single Responsibility to hard. (I read again The Pragmatic Programmer recently). (Sorry for the "buzzwords") In Java, I used to compose my classes using Dependency Injection then I gain in Better Readability, Testability and Decouple... But I lose in too much indirection... I didn't see those advantages in Ruby. But I like this idea of testability and composition. \$\endgroup\$ – Cristian Oliveira Sep 29 '15 at 22:31
  • \$\begingroup\$ @CristianOliveira I expanded the answer to clarify somewhat hopefully. \$\endgroup\$ – Borsunho Sep 30 '15 at 7:30
  • \$\begingroup\$ Thanks again! I see what you mean and I really like that. Then I do not need a Factory anymore... cause I can include Translator on Rufregle by mixin. About the couple between Extractor and Requester totally agreed. \$\endgroup\$ – Cristian Oliveira Sep 30 '15 at 16:24
1
\$\begingroup\$

It's hard to say much about such a short code sample, but factories simply encapsulate object creation (wiring up objects), which frees code using them from that responsibility. In that sense, factories are relevant in ruby too.

However, note that client code would look like this, given your current code:

translator = FreeGoogle::Factory.create

which isn't as readable as it could be. I think I'd prefer to write something like the following in my client code:

translator = FreeGoogle.new_translator
\$\endgroup\$
  • \$\begingroup\$ Thanks @Jonas spent your time reading the code! I see your point. That make sense... In my actual code I am factoring but Is not clear what kind of "product" I am making. \$\endgroup\$ – Cristian Oliveira Sep 29 '15 at 22:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.