Skip to main content
Code Review

Return to Question

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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

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

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

Tweeted twitter.com/#!/StackCodeReview/status/343368828228599808
added 928 characters in body
Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90

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.


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.

Source Link
Flambino
  • 33.3k
  • 2
  • 46
  • 90

define_method vs method_missing and CodeClimate scores

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?

lang-rb

AltStyle によって変換されたページ (->オリジナル) /