2
\$\begingroup\$

Can someone please review my code:

# dup/ish -> //github.com/rails/rails/issues/5449

class Form
  include Mongoid::Document

  attr_accessor :source

  def self.ish(prefix, suffix, &block)
    klass_name = prefix.classify + suffix

    dup.tap do |klass|
      klass.class_eval do
        define_singleton_method(:name) do
          prefix.classify
        end
      end

      klass.class_eval(&block)

      Object.const_set(klass_name, klass)
    end
  end

  def self.compose(klass_name, id, source)

    mongoid_translate = {:models => {}, :attributes => {}}

    form = ish(klass_name, id) do

      translate = {}

      source.flatten.each do |widget|
        if widget.include?('validate')
          validates *widget.values_at('name', 'validate')
          translate.store *widget.values_at('name', 'translate')
        end

        if widget.include?('name')
          if 'upload' == widget['type']
            has_many(:attachments)
            accepts_nested_attributes_for(:attachments)
          elsif 'repeat' == widget['type']
            part_klass = Form.compose(widget['name'], id, widget['parts'])
            embeds_many(widget['name'].pluralize, :class_name => part_klass.class.to_s)
            accepts_nested_attributes_for(widget['name'].pluralize)
          end
        end
      end

      mongoid_translate[:attributes][self.name.downcase.to_sym] = translate
    end

    I18n.backend.store_translations(:pt_BR, {:mongoid => mongoid_translate})

    form.new.tap do |form|
      form.source = source
    end
  end

  def prepare
    self.source.flatten.each do |widget|
      if widget.include?('name')
        if 'repeat' == widget['type']
          parts = send("#{widget['name'].pluralize}")

          if parts.empty?
            parts.build

            widget['parts'].each do |part|
              parts[0].write_attribute(part['name'], nil)
            end
          end
        elsif 'upload' == widget['type']
          attachments.build if attachments.empty?
        else
          begin
            send(widget['name'].to_sym)
          rescue NoMethodError
            write_attribute(widget['name'], nil)
          end
        end
      end
    end
  end
end
\$\endgroup\$
2
\$\begingroup\$

Few things I have noted in your design are listed below. Beware all of this is purely from a design perspective rather than application.

  1. Source is defined as attr_accessor which essentially violates encapsulation. Inject it from outside in constructor call.(I am not able to understand why you have not gone for attr_reader instead of accessor.
  2. Ideally self.compose can be split into different method calls. It is violating SRP by doing too many things at one place. That is same for other methods as well.
  3. Speaking from a puritan view, the branched if condition is also a smell.

In general, metaprogramming is considered to be something which must be avoided unless you have enough justification to keep it.

\$\endgroup\$
  • \$\begingroup\$ Whoa I totally forgot I put this here. :P And yes I see very well all your points. It's actually something I knew had smells all over the place but wanted to check with other people (which I couldn't because I was the only programmer working on the project). Thanks for your time and effort! Have a great day. \$\endgroup\$ – Arthur Corenzan Jan 7 '16 at 17:44
  • \$\begingroup\$ my pleasure! there might be more smells. \$\endgroup\$ – Anony-mouse Apr 14 '16 at 9:59

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.