Hi, Good post. However I don't agree with it :-).
Note that I am not against using lambdas, but rather think that Wicket core should not provide lambda extension points for component/behavior life cycle methods. Comments inline and discussion at the end of this message. On Tue, Feb 21, 2017 at 3:36 AM, Andrew Geery <[email protected]> wrote: > 1. I don't think we should consider every onXXX as a candidate for lambda > usage. Specifically, I don't think Component-level methods like > onInitialize(), onConfigure(), onDetach(), etc. should be candidates > because overriding those methods is fairly rare. Perhaps in your development, but I just crafted some statistics across our projects and see the following statistics: Biggest project (1,301,269 lines of Java code): onInitialize: 906 onConfigure: 1029 onBeforeRender: 366 onComponentTag: 65 onAfterRender: 6 onDetach: 1524 onEvent: 118 onClick: 1611 onSubmit: 814 Smaller project (1,069,352 lines of Java code): onInitialize: 194 onConfigure: 832 onBeforeRender: 153 onComponentTag: 81 onAfterRender: 10 onDetach: 1400 onEvent: 56 onClick: 1788 onSubmit: 805 Smallest project (39,162 lines of Java code): onInitialize: 40 onConfigure: 76 onBeforeRender: 6 onComponentTag: 40 onAfterRender: 4 onDetach: 56 onEvent: 13 onClick: 105 onSubmit: 67 In-house Framework used by above applications (173,241 lines of Java code): onInitialize: 45 onConfigure: 206 onBeforeRender: 79 onComponentTag: 91 onAfterRender: 4 onDetach: 43 onEvent: 14 onClick: 139 onSubmit: 70 As you can see, the number of times onConfigure, onBeforeRender, onInitialize are overridden is in the same ballpark as onClick/onSubmit. It is not an order of magnitude higher. > For example, if I > override those methods, I am almost always in a standalone sub-class of > Panel; it is unusual for me to override them for things like Link or Button > components. Indeed, to deal with concerns like visibility or being enabled > (common reasons to override #onConfigure()), I add a Behavior to control > this rather than sub-class the Component. Adding a behavior is nice if you have no memory restrictions. Adding a Lambda is nice if you don't have memory restrictions. Inheritence is a very efficient way of extending behavior and not consuming much memory. This is why we override onConfigure/onBeforeRender/onComponentTag when we already have to override onClick. You already have a subclass, you already have the model so make use of it. > 2a. I think what makes a method a good candidate for a lambda is where a > little more _specific_ functionality needs to be added to a component to > make it useful/complete. In my mind, the Link, AjaxLink and > AjaxFallbackLink components are quintessential examples of this: the > developer needs to add one (usually small) piece of functionality (the > onClick(...) method) to make the component useful. Nobody is denying that onClick/onSubmit are useful methods to implement, however, we are questioning the added API complexity, added memory and computing inefficiency introducing lambda's for these events, when you typically (see the stats above) also need to implement other functionality on the component as well. The only argument I see for improved readability when enabling lambda's is passing method references as the lambda. Even if you only have two lines of code in your onClick handler, the benefits of a lambda are negligible over just subclassing and overriding onClick. > 3. A major concern with introducing lambda expressions is to not penalize > components space-wise that don't use lambdas -- e.g., existing > functionality that sub-classes AjaxLink to override the onClick(...) method > should not have to store a null pointer to an unused lambda reference Also don't count out the computational burden of having to look up the unused lambda reference when no override is present and no lambda was attached. > 4. In the Component class, it is possible to store and access typed > "meta-data" with a component instance (#setMetaData/#getMetaData). If the > lambda handler is stored as meta-data, storage for the lambda will only be > allocated if a lambda handler is actually assigned. If a lambda handler is > not used, no storage for the handler would be allocated. We have considered that in our discussion. This requires performing the lookup, and when enabling the use of lambdas in this manner people will start using it (jay!), but memory consumption will rise dramatically. > 4. Make AjaxLink non-abstract and implement onClick(AjaxRequestHandler) by > calling getMetaData(AJAX_CLICK_HANDLER_KEY) and calling the #handleClick > method on the lambda with the AjaxRequestTarget if the lambda is not null. This will fail with the default code completion template of Eclipse that inserts a super call for all overridden methods. It conflicts with Wicket's other lifecycle methods where you are required to call super (onConfigure, onInitialize, onDetach) in that in this case you should not call super. Also this breaks our explicit contract where we require users to implement the onClick method. It directs their use of our API and is practically self-documenting. By making (Ajax)Link non-abstract and having an implementation for onClick() we loose this. Furthermore when added to Wicket Core, we introduce a second way to implement behavior in our API: - use inheritance - use lambda event handlers where the use of lambda event handlers will come with some caveats: - don't use for components where there are many instances of - don't override the onClick, or don't call super.onClick - components that have onClick overridden and a onclick lambda handler is registered - components that have a onclick lambda handler registered and have onClick overridden > 5. Because AjaxClickHandler is a @FunctionalInterface, the setClickHandler > method can be invoked with a lambda (and without explicitly referencing the > ClickHandler interface at all): > > new AjaxLink("id").setClickHandler(target -> target.add(myLink)); OK, and now I need to change the visiblity dynamically, so I implement (according to our current idioms): new AjaxLink("id") { @Override protected void onConfigure() { super.onConfigure(); setVisibilityAllowed( some condition ); } }.setClickHandler(target -> { do some action target.add(component); }); What have we gained? > The advantage of this is: > > 1) Usages of AjaxLink that do not use lambdas do not take up any more space > than they did before; and > 2) It is still possible to sub-class AjaxLink (e.g., to implement > #onConfigure). > > The disadvantage of this is that, as Martijn pointed out, it doesn't make > sense to call setClickHandler() on a subclass where onClick has been > overridden. However, because we're adding lambda handlers to a relatively > small number of components, and adding it to implement a very specific > piece of functionality (i.e., onClick is much more specific than > onConfigure), I don't think this is that confusing. I posit that it *is* confusing, because you now have two ways of accomplishing the same task, where one does not bring many benefits over the other, but rather restricts and directs developers into the wrong direction. I can see the pull requests coming where we should add "setConfigureHandler", "setBeforeRenderHandler", "setEventHandler", "setInitializeHandler", etc. -- Become a Wicket expert, learn from the best: http://wicketinaction.com
