2
\$\begingroup\$

This may be a better fit for another board, but here goes:

I've made a very simple gem that acts as a days-of-the-week bitmask. To be hip, I registered it on CodeClimate, where it's currently scoring a B/3.0 grade.

Since there's only 1 extremely simple class, the B is bugging me. CodeClimate's reason is that there's too much code outside methods. This is no doubt due to my use of define_method to programmatically add read/write accessor methods like monday, tuesday and so on to the class.

The methods are currently being added like so:

DAY_NAMES = %w(sunday monday tuesday wednesday thursday friday saturday).map(&:to_sym).freeze
DAY_BITS  = Hash[ DAY_NAMES.zip(Array.new(7) { |i| 2**i }) ].freeze

# ...

# Create the day=, day, and day? methods
DAY_BITS.each do |day, bit|
  define_method(day) do
    get_bit bit
  end
  alias_method :"#{day}?", day

  define_method(:"#{day}=") do |bool|
    set_bit bit, bool
  end
end

Note that the class supports two reader syntaxes (with and without ?).

Now, I could use method_missing instead to make CodeClimate happy, but as I see it, that's only more complex. The method names are all known in advance, so method_missing seems unnecessary, and much less efficient than defining the methods on the class once.

If I were to use method_missing I'd do something like this (get_bit and set_bit already accept both strings and symbols):

DAY_NAME_PATTERN = "((?:sun|mon|tues|wednes|thurs|fri|satur)day)".freeze

# ...

def method_missing(method, *args)
  case method.to_s
  when %r{\A#{DAY_NAME_PATTERN}\??\Z}
    get_bit $1
  when %r{\A#{DAY_NAME_PATTERN}=\Z}
    set_bit $1, args.first
  else
    super
  end
end

I'll still need the DAY_NAMES and DAY_BITS constants, so DAY_NAME_PATTERN would be yet another const - one that seems somewhat like duplication (though I could build it from DAY_NAMES of course, though that would be more expressions outside methods).

So to my mind, using method_missing would only improve the CodeClimate score, yet (ironically) make the code worse. Would you agree? Or do you know some clever 3rd way?


Aside: As Tokland rightly notes in his answer a bitmask is a fairly low-level approach to all of this compared to using sets.

I'm using a bitmask because the class is primarily intended to be used as an ActiveRecord serializer in a Rails app. That's where I extracted it from (partly as an exercise for myself). In the case of my app, the database already had a bitmask column, and this was an encapsulation of that logic. It seemed to me a good way to store the value (even if it is a little arcane for Ruby), as it allows for fast but complex day intersection/union queries at the SQL layer.

All of that could still be accomplished using sets, and only calculating the bitmask when necessary. Regardless of the specifics, however, my question is still valid on its own. Tokland's answer is great, but not quite an answer to the specific question.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Notes:

  • I am afraid I'd propose a very boring solution: don't use meta-programming. I mean, why add methods and more methods, polluting the class, when you can add just one method and use symbols for the days? Add validations if you're worried about invalid values for days.

  • The usual trick of using powers of two as a mask is that, a trick, very handy in low-level languages (C, assembler, ...). In a higher-level language you strive for writing code as declarative as possible, with data structures like lists, hashes, trees, or sets, not masks of powers of 2 :-) Here I'd use a set.

  • I prefer functional solutions even with OOP, just return new instances instead of modifying the existing ones. Referential transparency, idempotence, all the goodies follow.

I'd write (orientative, you get the idea):

class WeekSauce
  DAY_NAMES = [:sunday, :monday, :tuesday, :wednesday, :thursday, :friday, :saturday]

  def initialize(days)
    @days = days.to_set & DAY_NAMES
  end

  def has_day?(name)
    @days.include?(name)
  end

  def add_days(names)
    WeekSauce.new(@days.union(names))
  end
end
\$\endgroup\$
3
  • \$\begingroup\$ I see your point, however: This class is intended to be used as an ActiveRecord serializer. It was extracted (mostly for the heck of it) from a Rails app, and serializes to a simple integer column. Furthermore, the bitmask is sent as-is to JSON API consumers (which seems more precise and easier for any language to devour, than sending an array of stringified symbols). I agree it'd be a lot more straight forward to just use symbol arrays, but it started as an int bitmask thing. So this class is, in a way, the high-level data structure. I could do everything different, but... eh \$\endgroup\$
    – Flambino
    Commented Jun 7, 2013 at 21:33
  • \$\begingroup\$ Fair enough, but sets or not sets, the main point of my answer (apart from data-structures and FP) was that IMHO grouping methods by task instead of adding individual methods simplifies the API. If you want to add them, well, I don't think there is a better way that what you did (the first, definitely not method_missing, no matter what a linter says) \$\endgroup\$
    – tokland
    Commented Jun 9, 2013 at 9:24
  • \$\begingroup\$ Gotcha. As I said, I partly extracted the gem for the heck of it. I agree I probably went overboard on the API, but I wanted it flexible as possible. Not a great way of doing things compared to having a more focussed API, but that's where the "for the heck of it" comes in. In any case, you've answered the main question: Given the circumstances method_missing would be worse than what I did, CodeClimate's opinion notwithstanding. Thanks! \$\endgroup\$
    – Flambino
    Commented Jun 9, 2013 at 15:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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