How far to go in replacing inner classes with lambdas?

[Sam Pullara]

On Oct 28, 2012, at 10:18 AM, Remi Forax <forax at univ-mlv.fr> wrote:
> On 10/28/2012 05:53 PM, Joshua Bloch wrote:
>> I would stop at the first refactoring: I'm not convinced that the unbound method reference construct will /ever /look familiar. That said, neither of the refactorings is as easy to understand as the original code declaration. In the first, you loose the fact that the argument to an Executor's sole abstract method is of type Runnable. When you're dealing with two SAM types at once (Executor and Runnable in this case), things can get a bit confusing. The second refactoring does make the argument type (Runnable) explicit, but it's just unreadable.

I agree with the unbound method reference being very hard to read — not sure how long it will take me to get used to them. I'm ok with the Runnable type being lost mostly because it isn't that interesting in this case since you don't do anything with it. 

>> 
>> A casualty in both refactorings is the comment:
>> 
>>          // DirectExecutor using caller thread

Agree.

>> 
>> It's invaluable in keeping the code readable, and must be maintained.
>> 
>>    Josh
>> 
>> P.S. While you're cleaning up the code, "final static" should be "static final."
> 
> Brian,
> you can go a step further because either r -> r.run() or Runnable::run are non capturing lambdas
> thus they doesn't need to be stored in a static final field.
> So you can replace
> 
>    public NotificationBroadcasterSupport(Executor executor, MBeanNotificationInfo... info) {
>      this.executor = (executor != null) ? executor : defaultExecutor;
>      ...
>    }
> 
> by
> 
> public NotificationBroadcasterSupport(Executor executor, MBeanNotificationInfo... info) {
>      // if the executor is null, the runnable is executed by the current thread
>      this.executor = (executor != null) ? executor : r -> r.run();
>      ...
>    }

This is pretty nice that we no longer have to have separate fields for stuff like this. Also, putting the lambda in context rather than in a field, I think makes it much more clear what is going on. Almost don't need the comment with this code.

Sam

> 
> You can apply the same idea to j.u.functions.Mappers and Predicates.
> 
> cheers,
> Rémi
> 
>> 
>> On Sun, Oct 28, 2012 at 9:12 AM, Brian Goetz <brian.goetz at oracle.com <mailto:brian.goetz at oracle.com>> wrote:
>> 
>>    The IntelliJ EAP builds already have inspections for "replace
>>    inner with lambda" and "replace lambda with method reference"
>>    (nice job, guys!)
>> 
>>    In browsing through the inspection results in the JDK, we find
>>    ourselves in the same position we did with Coin features like
>>    Diamond; many code snippets can be simplified, but not all code
>>    snippets that can be simplified should be.
>> 
>>    Here's an example:
>> 
>>        private final static Executor defaultExecutor = new Executor() {
>>                // DirectExecutor using caller thread
>>                public void execute(Runnable r) {
>>                    r.run();
>>                }
>>            };
>> 
>>    We could simplify this to:
>> 
>>        private final static Executor defaultExecutor = r -> r.run();
>> 
>>    or further to
>> 
>>        private final static Executor defaultExecutor = Runnable::run;
>> 
>>    (Executor is a SAM interface with one method, execute(Runnable).)
>> 
>> 
>>    What guidelines can we offer users on when they should and should
>>    not replace inner classes with lambdas or method reference?  Does
>>    this last one look a little weird only because the powerful
>>    "unbound method reference" construct is still a little unfamiliar?
>> 
>> 
>