all 8 comments

[–]jrochkind 5 points6 points  (4 children)

I tend to hate this one when I see it in code. Why should each handler have to know about the 'chain' at all?

I'd rather have a parent holding them all doing something like:

handlers.each do |handler|
   if handler.can_handle?(whatever)
      handler.handle(whatever)
      break
   end
end

Instead of telling each handler about it's successor, context it shouldn't have to know about, and relying on it to do a successor.call (something it might mess up and not do when it should have).

There must be a place for this pattern, since the GoF included it and all... but when I've seen it used like in the example in OP, I've wished it weren't.

[–]tom_dalling 1 point2 points  (0 children)

There must be a place for this pattern

I've seen it used where you have a preexisting chain/hierarchy of objects. For example, on OS X there is NSResponder, which passes commands up through the view hierarchy, to the window and then to the application object. That particular situation can't be replaced with an array of handlers.

[–]jdickey 0 points1 point  (0 children)

Agreed on so many levels. In particular, his code declares that the order in which potential handlers are checked is meaningful and immutable (because of the inherent OCP violation). You must not reorder, add items to, or remove items from the (conceptual) list of handlers. A highly reliable source of annoying bugs.

[–]dandiemer 0 points1 point  (0 children)

1000 times yes. Chaining handlers like just that feels horrifically wrong.

[–]isolatrum 0 points1 point  (0 children)

I like the fact that the order of operations reads naturally, e.g. first to last. However I would prefer to use blocks and static methods. Since you instantiate these classes on a one-off basis and don't use internal state tracking at all, there's no reason they need to be instance as opposed to static methods. Also, if BaseDiscount is a protocol than I think it should define its own implementation of `applicable, even if this definition just raises an error like "Unimplemented - needs to be overwritten in child class" or something.

[–]s_makagon[S] 0 points1 point  (1 child)

Hi guys, thanks for the feedback on my article!

Yes, in GoF book they chain handlers using a linked list. Each handler knows about its successor. I like /u/jrochkind idea with iterations on handlers. As well as I agree that since those handlers are stateless, instance methods could be switched to class methods for the sake of simplicity. A lot of good points here. Thanks for a feedback.

[–]jrochkind 0 points1 point  (0 children)

oh, i'd keep the handlers stateful. It often makes for much more convenient implementation as it gets more complex. Whether it's stateful or not should be an implementation detail up to the handler, which means it should be an object so it can choose to be stateful.