On Oct 25, 2010, at 11:23 AM, Les Hazlewood wrote: >>> But I'm not sure that inheritance is used too often. Sure, it is used >>> in some places where it shouldn't be and that there are definitely >> >> Side comment, it's used all over the place. Filters, etc. > > Inheritance is not bad - it's just that composition should generally > be favored over it. The Filter hierarchy, while deep, has rarely been > refactored in the last few years. This particular example actually > demonstrates where it can make sense. Let me restate: Composition > Should Be Favored Over Inheritance. I know this. But we also have a > 6+ year-old code base that can't be changed overnight and some parts > of it are probably just fine the way they are. > > Now if we want to change the remaining places where this exists for > 2.0, then great - let's talk about that.
Yeah, just a chat as I get my head around more of this stuff. Not everything, if anything, should see the light of day. For me it's just interesting to talk to people about how to structure a security API. >> Let's look at the DefaultSecurityManager. DefaultSecurityManager extends >> SessionsSecurityManager, SessionsSecurityManager extends >> AuthorizingSecurityManager, AuthorizingSecurityManager extends >> AuthenticatingSecurityManager, AuthenticatingSecurityManager extends >> RealmSecurityManager, and, finally, RealmSecurityManager extends >> CachingSecurityManager. >> >> I'm guessing there's over 1200 lines of code there spread across six classes. >> >> All this for wrapper classes. >> >> Don't get me wrong. I'm all for delegation. What I am against is the long >> chain of inheritance. The above inheritance list reads like Matthew chapter >> 1. ;) Why such a large totem pole? I haven't groked the entire set of >> code but I suspect there's some intimate collaboration there. If there >> isn't then they really should be broken up. > > You've just complained about it without actually looking at the source > code, which to me is a bit presumptuous. While your point may be > justified (and probably is!), it's a little arrogant to make > conclusions from assumptions without doing some homework first. I > respect your opinion Alan - this just felt a little off to me - sorry > if I took it too personally. Nope. My bad. As I mentioned before this is my first real run through the code and I should have left out the jokes. This stuff is great and I've been able to do a lot of great stuff with the code base exactly as it is with little or no work what so ever. I'm just getting my head around it. Again, I should have left out the jokes. It definitely does not reflect my opinion of the code base. Please accept the apologies from this armchair quaterback! > Anyway, there is no intimate collaboration between subclasses - not > until you get to DefaultSecurityManager and DefaultWebSecurityManager, > so theoretically, all of the parent classes can be moved to > DefaultSecurityManager and still be backwards-compatible. > > The hierarchy that exists today exists for logical grouping of code to > make it a little more manageable: when you're editing code in the > SessionsSecurityManager subclass, you're only worrying about dealing > with the nested SessionManager. When you want to work with the > Authorizer, you only touch the AuthorizingSecurityManager subclass, > etc. > > It is purely a logical grouping mechanism so we as developers don't > get swallowed under the 1500 or 2000 lines of code that would exist in > a single class otherwise. Now if this is something we as a team think > we should change and move it all to DefaultSecurityManager, then I > think we can do that. It would be a huge class though ;) > >> They should be broken up anyway. >> >> Take this with a grain of salt; as I mentioned before I don't grok the >> entire set of the above code. > > I think you'll find that I and others on this list are very open and > helpful and we try to help people understand things whenever we can - > and I often do this in great detail (maybe even to the chagrin of most > readers). All it takes is a quick question to learn something. > (Passing informed judgement and suggesting improvements is fine by the > way, and I personally welcome it because it will help this project be > better). :) > >> The managers should be separate final entities. Collaborative behavior >> could be specified by wiring in various listeners between them. The >> DefaultSecurityManager could do this. > > The Managers are separate entities - the SecurityManager > implementation acts as an umbrella for all of them, delegates to them > as necessary, and additionally performs some logic related to Subject > management. We could have a SubjectManager that did this logic and > then the SecurityManager implementation would be nothing more than a > container of the others. > > We can refactor this stuff and I have no issues with that. But the > pinnacle feature of the framework is usability and simplicity above > all else - even if it causes some extra work for the development team. > When an end-user instantiates the SecurityManager implementation and > provides at least one Realm - that's it - they're good to go. > Everything works out of the box. > > I personally would like to keep this usage scenario and definitely > want to avoid the user having to know how to wire up all the > individual instances themselves. Now we could do this through a > Builder, which I've thought about more than a few times in the past, > but never got around to it because no-one was complaining that the > SecurityManager implementations were causing anyone problems in > practice. > > Also understand that the SecurityManager wrapper exists as an ideal > handle for INI-based IoC configuration. We'd have to think about how > changing anything would affect that as well. I think that I wandered into some minutiae when I was making my inheritance points. I'm going to take these bits around the block a few times before I venture any more comments on it. :) Regards, Alan
