views:

713

answers:

4

I am still very new to Ruby (reading through the Pickaxe and spending most of my time in irb), and now that I know it's possible to patch classes in Ruby, I'm wondering when it's acceptable to do so, specifically whether it's acceptable to patch Ruby's base classes. For example: I answered another Ruby question here where the poster wanted to know how to subtract hours from a DateTime. Since the DateTime class doesn't seem to provide this functionality, I posted an answer that patches the DateTime and Fixnum classes as a possible solution. This is the code I submitted:

require 'date'

# A placeholder class for holding a set number of hours.
# Used so we can know when to change the behavior
# of DateTime#-() by recognizing when hours are explicitly passed in.

class Hours
   attr_reader :value

   def initialize(value)
      @value = value
   end
end

# Patch the #-() method to handle subtracting hours
# in addition to what it normally does

class DateTime

   alias old_subtract -

   def -(x) 
      case x
        when Hours; return DateTime.new(year, month, day, hour-x.value, min, sec)
        else;       return self.old_subtract(x)
      end
   end

end

# Add an #hours attribute to Fixnum that returns an Hours object. 
# This is for syntactic sugar, allowing you to write "someDate - 4.hours" for example

class Fixnum
   def hours
      Hours.new(self)
   end
end

I patched the classes because I thought in this instance it would result in a clear, concise syntax for subtracting a fixed number of hours from a DateTime. Specifically, you could do something like this as a result of the above code:

five_hours_ago = DateTime.now - 5.hours

Which seems to be fairly nice to look at and easy to understand; however, I'm not sure whether it's a good idea to be messing with the functionality of DateTime's - operator.

The only alternatives that I can think of for this situation would be:

1. Simply create a new DateTime object on-the-fly, computing the new hour value in the call to new

new_date = DateTime.new(old_date.year, old_date.year, old_date.month, old_date.year.day, old_date.hour - hours_to_subtract, date.min, date.sec)


2. Write a utility method that accepts a DateTime and the number of hours to subtract from it

Basically, just a wrapper around method (1):

def subtract_hours(date, hours)
  return DateTime.new(date.year, date.month, date.day, date.hour - hours, date.min, date.sec)
end


3. Add a new method to DateTime instead of changing the existing behavior of #-()

Perhaps a new DateTime#less method that could work together with the Fixnum#hours patch, to allow syntax like this:

date.less(5.hours)


However, as I already mentioned, I took the patching approach because I thought it resulted in a much more expressive syntax.

Is there anything wrong with my approach, or should I be using one of the 3 alternatives (or another one I haven't thought of) in order to do this? I have the feeling that patching is becoming my new 'hammer' for problems in Ruby, so I'd like to get some feedback on whether I'm doing things the "Ruby way" or not.

+4  A: 

Personally, I think it's acceptable to add methods to the base classes, but unacceptable to modify the implementation of existing methods.

Don
I second this opinion.
Gordon Wilson
The issue here is that one person's addition is another person's conflict. I can't tell you how many times I've seen a (slightly different) implementation of `String#/()`.
Avdi
If everyone thinks they can add methods to a base class, then you get conflicts as people do this in parallel. It might not matter when you control all the source, but when you libraries that don't control, things get really weird.
brian d foy
A: 

I think its like this: If you honestly feel that most other programmers would agree with your patches, then fine. If not, perhaps you should instead be implementing a code library?

dicroce
I wasn't really planning on pushing my patches on the Ruby community at large. I was assuming this would be a local change.
Mike Spross
+4  A: 

The safest way is to define your own class that inherits from the built-in one, then add your new stuff to your new class.

class MyDateTime < DateTime
  alias...
  def...

But obviously now you only get the new behavior if you declare objects of your new class.

glenn mcdonald
+1 That was obvious enough for me not to think of it ;-)
Mike Spross
Don't forget delegation/decoration: http://avdi.org/devblog/2008/04/17/sustainable-development-in-ruby-part-3-delegation/
Avdi
+14  A: 

My personal answer, in a nutshell: the core-class patching hammer should be at the bottom of your toolbox. There are a lot of other techniques available to you, and in almost all cases they are sufficient, cleaner, and more sustainable.

It really depends on the environment in which you are coding, though. If it's a personal project - sure, patch to your heart's content! The problems begin to arise when you are working on a large codebase over a long period of time with a large group of programmers. In the organization I work for, which has Ruby codebases of over 100KLOC and and twenty or so developers, we have started to crack down pretty hard on monkey patching, because we've seen it lead to head-scratching, man-hour wasting behavior far too often. At this point we pretty much only tolerate it for temporarily patching third-party code which either hasn't yet incorporated or won't incorporate our source patches.

Avdi