0

I have the following models:

module Core
  class Conditioner
    include Mongoid::Document

    field :operator, type: String, default: '' # can be !=, ==, <, >, <=, >=, =
  end
end

module Core
  class Count < Conditioner
    field :threshold, type: Integer, default: 0 # a simple threshold
  end
end

module Core
  class Time < Conditioner
    UNITS = %w(seconds minutes hours days weeks months years)

    # will be use like this: Time.value.send(Time.unit) Ex: 3.minutes
    field :value, type: Integer, default: 0    # 3
    field :unit,  type: String,  default: ''   # minutes

    validates :unit,  presence: true, inclusion: { in: UNITS }
  end
end

Was wondering if I should namespace the Count and Time class with Conditioner ? Like this:

module Core
  class Conditioner::Time < Conditioner
  end
end

Since I have to call Time.now like this now ::Time.now.

EDIT

Regarding answers, maybe this should be a better idea:

module Core
  module Conditioner
    class Base
    end
  end
end

module Core
  module Conditioner
    class Count < Conditioner::Base
    end
  end
end

module Core
  module Conditioner
    class Time < Conditioner::Base
    end
  end
end

Since defining a class called Core::Time is maybe too generic and does not make many sense.

What do you think? Not sure about the best practice here.

Pierre-Louis Gottfrois
  • 17,561
  • 8
  • 47
  • 71

3 Answers3

1

You don't have to namespace it, although you can if you want to.

Whether you should or shouldn't depends on what do you want to model (and not whether you have to address Time with ::Time or not...). It looks like your Core::Time already subclasses Core::Conditioner, so it doesn't make much sense to make it an inner class of it's superclass. In your case it is better not to namespace it.

Having the same class name as ruby is not an issue here, since you have already namespaced it with Core.

ajt
  • 553
  • 8
  • 25
jurglic
  • 3,599
  • 1
  • 17
  • 13
  • Thanks for the answer, what you say is interesting, what about the edit ? – Pierre-Louis Gottfrois Aug 14 '13 at 08:02
  • It is hard to say, since I don't know the domain you are modelling. Can you describe what each does class in a sentence, and how they work together? – jurglic Aug 14 '13 at 12:47
  • Sure. The `Core` namespace is only here since I'm on an isolated Rails Engine. Basically I want to have 2 type of conditions in my application. A "count" base condition and a "time" base condition. They will share common methods and attributes but also have their own. Count condition check whether a given X is greater than `Count.threshold`. Time condition check whether a given X date is greater than `Time.value.send(Time.unit)` which translate in `3.minutes` for example. I edited my question with relevant details. – Pierre-Louis Gottfrois Aug 14 '13 at 14:14
  • 1
    I think both are completely viable approaches and the differences quite are negliable. The first one has less naming/nesting but you get somewhat less descriptive classnames (Core::Count vs Core::Conditioner::Count). You could also try something in the direction of Core::CountCondition and Core::TimeCondition... but again at this point differences are negliable. – jurglic Aug 14 '13 at 18:33
0

In my opinion, Don't use a class name that Ruby or Rails has used, it may be a problem. You must be careful whenever you use it. I think it is unnecessary. Don't use namespaces unless you have many models to be maintained. Keep it simple and the maintenance will be easier :)

Bigxiang
  • 6,252
  • 3
  • 22
  • 20
  • Since `Count` and `Time` have their context only used with `Conditioner` does is not make sense to namespace it ? – Pierre-Louis Gottfrois Aug 14 '13 at 08:03
  • There is not the restriction of namespaces, you can namespace it or you can give your class an unique name with a prefix, like CondCount and CondTime, because there are just 2 classes, then you can use Time.now anywhere. Reduce the complexity and not make names confused. – Bigxiang Aug 14 '13 at 08:20
0

The only thing you change by prefixing Conditioner to Time is that the constants lookup for the Core module will not see your Conditioner::Time class when looking up Time. All the other classes (Count, Conditioner) will still think Conditioner::Time is superseding Time.

module Core
  def time
    Time.now
  end
  class Conditioner
    def time
      Time.now
    end
  end
end

module Core
  class Count < Conditioner
    def time
      Time.now
    end
  end
end

module Core
  class Conditioner::Time < Conditioner
    def time
      Time.now
    end
  end
end

Any call like Core::Count.new.time will fail, but

class A
  include Core
end

A.new.time will put out the current time, while in this case:

module Core
  def time
    Time.now
  end
  class Conditioner
    def time
      Time.now
    end
  end
end

module Core
  class Count < Conditioner
    def time
      Time.now
    end
  end
end

module Core
  class Time < Conditioner
    def time
      Time.now
    end
  end
end

class A
  include Core
end

A.new.time will also fail. Now, the reason why is fairly easy: The constants lookup always looks at the constants available in the current namespace (Core), then climbs up the ancestor chain of the current namespace before looking in the namespace of Object (Time can also be called as Object::Time. It won't search in child namespaces (which you create by putting Conditioner:: before Time) though, and that's why renaming your class Time to Conditioner::Time only changes lookup for the Core module, but not for the classes within Core.

So, asking for best practice in this case: Just leave it at calling your class Time and referencing to Object::Time by ::Time. This is a fairly well known practice, and the benefits of additional namespaces to avoid using it are marginal.

Beat Richartz
  • 9,474
  • 1
  • 33
  • 50