>> 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. > 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. 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. Best, Les
