Defining functions

[Reinmar]

Proposal for defining functions in the code. Should be added to https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style.

1. Use arrow functions for anonymous functions (callbacks):

   describe( 'foo', () => {
       it( 'bars', done => {
           foo.bars( x => x + 1, done );
       } );
   } );
   

2. When using arrow functions, make sure to wrap the body of the function with {} to clarify where it ends if that may not be clear:

   // OK. Super clear.
   foo( x => x() + 1 );
   
   // BAD. May be confusing what's going on there.
   Promise.resolve().then( () => this._creator instanceof Creator && this._creator.destroy(), catchMe );
   
   // OK.
   Promise.resolve().then( () => {
       return ( this._creator instanceof Creator ) && this._creator.destroy()
   }, catchMe );
   

3. When defining a function always use a function declaration. Usually you'll also want to keep the definition at the end of the outer function (not applying to tests).

   // OK.
   on( event ) {
       this._domNode.addEventListener( event, domListener );
       this._domListeners[ event ] = domListener;
   
       function domListener( domEvt ) {
           this.fire( event, domEvt );
       }
   }
   
   // BAD.
   on( event ) {
       var domListener = domEvt => this.fire( event, domEvt );
   
       this._domNode.addEventListener( event, domListener );
   
       this._domListeners[ event ] = domListener;
   }
   

4. Use named function expression (or a separate function declaration) when defining an important step of some algorithm to ease debugging:

   init() {
       return this.loadPlugins()
           .then( initPlugins )
           .then( fireCreator )
           .then( function implode() {
               //  ... lots of important code as well.
           } );
   
       function initPlugins( ... ) {
           // ... lots of important code.
       }
   
       function fireCreator( ... ) {
           // ... lots of important code too.
       }
   }
   

[Reinmar]

Please note that we already discussed and agreed about point 3. Points 1. and 2. needs to be defined after we made a decision to use ES6.

[pjasiun]

👍 Agreed. I would add one more thing:

Declaring function as assigning anonymous function to the variable is also a bad smell:

// OK.
on( event ) {
    this._domNode.addEventListener( event, domListener );
    this._domListeners[ event ] = domListener;

    function domListener( domEvt ) {
        this.fire( event, domEvt );
    }
}

// BAD.
on( event ) {
    var domListener = domEvt => this.fire( event, domEvt );

    this._domNode.addEventListener( event, domListener );

    this._domListeners[ event ] = domListener;
}

// ALSO BAD.
on( event ) {
    var domListener = funcion ( domEvt ) {
            this.fire( event, domEvt );
        };

    this._domNode.addEventListener( event, domListener );

    this._domListeners[ event ] = domListener;
}

[pjasiun]

Usually you'll also want to keep the definition at the end of the outer function.

And this should be a rule too, in my opinion.

[Reinmar]

We found one case when using a function expression makes more sense than using a function definition:

        _createDomListener( event ) {
            var domListener = domEvt => {
                this.fire( event, domEvt );
            };

            // Supply the DOM listener callback with a function that will help
            // detach it from the DOM Node, when it is no longer necessary.
            // See: {@link off}.
            domListener.removeListener = () => {
                this._domNode.removeEventListener( event, domListener );
                delete this._domListeners[ event ];
            };

            return domListener;
        }

I'll update the proposal to explain such case.

[Reinmar]

We follow the plan to use function declarations as often as possible and assigning function expressions to variables only if it makes real sense in a given context.