Extract security from Decorator

[haines]

I feel like Decorator is doing quite a number of different things, and perhaps some of it could be nicely extracted. The denies/allows logic seemed like a good place to start as it was a little bit hacky anyway.

[jcasimir]

Just looking at the diffs, I like it.

[nashby]

not sure, but looks like a module to me.

[jcasimir]

@nashby agreed, I was reading a bit fast. A module is the right conceptual move and would not need the proxy stubs in decorator.rb

[haines]

Yep, agreed, I'll change it.

[haines]

Yeah, you guys were right, this is nicer now. Let me know if you want me to rebase.

[nashby]

@haines awesome, thanks! Let's wait for @steveklabnik

[steveklabnik]

Oh no! When I saw this title, I was like "I bet it's a module, can I bother him to ask for it as a class?"

[steveklabnik]

Yes, I would prefer the class version very much. "Prefer composition over inheritance." Can you drop b5cc216?

[haines]

Haha oh dear. I liked the module version because it's less code, but I guess less code != better code :)

[steveklabnik]

Well, let's discuss! @jcasimir was swayed by @nashby. I can see both, to be honest. Don't remove that commit yet, let's figure this out.

[haines]

Looking at it again, I think I'm landing back on the class side of the fence.

I like that the class version has the denies et al. methods defined directly on Decorator, which means they remain documented in the right place. I find with the module-based approach that it can sometimes be difficult to work out where a method is added (more so in big projects like Rails, obviously).

Also, the class option hides the private bits properly - allowed, denied and strategy. If someone defined an unrelated self.strategy method on a decorator, that would break the module-based one, right?

[steveklabnik]

Modules

1. are inheritance, and therefore, should be used sparingly.
2. can't be instantiated, and can therefore be awkward to test. Object.new.extend(Security) and all that.
3. Are great for representing cross-cutting concerns.
4. are over-used by Rubyists.

Classes

1. are composition, which we should favor
2. have a bit more boilerplate than modules
3. are super simple to test
4. are not always included, so they're easier to test objects they're included in. Think validations, for example: always a pain, because they're always there.

So, which should we use?

Note that a DecoratedEnumerableProxy also can have methods now, so security should probably be on it, too. Which means that even though we're using it in one place here, it might be two, which means a module may make sense.

[stevenharman]

When I saw this title, I was like "I bet it's a module, can I bother him to ask for it as a class?"

I've developed the same knee-jerk reaction. Overuse of modules for "separation of concerns" has become so common that we can name it: http://stevenharman.net/bag-of-methods-module-and-grep-driven-development

[steveklabnik]

I wrote about this too: http://blog.steveklabnik.com/posts/2012-05-07-mixins--a-refactoring-anti-pattern

That said, I also talked to @nashby, and he is 👍 on classes too. So let's do it.

[stevenharman]

Ah yes... I remember that post. Well done. And 👍 to @nashby for moving forward with this in the first place.

[haines]

Sweet as, should be good to merge!

@stevenharman I love "grep-driven development"

Edit: in case that was ambiguous, I love the term "grep-driven development". I do not love grep-driven development itself :)

[steveklabnik]

❤️