Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider factory functions #24

Closed
SaulDoesCode opened this issue Jul 18, 2018 · 6 comments
Closed

Consider factory functions #24

SaulDoesCode opened this issue Jul 18, 2018 · 6 comments
Labels
improvement Something could be improved

Comments

@SaulDoesCode
Copy link
Contributor

Instead of using classes, consider using factory functions instead because then there would be no need for new and also by avoiding the use of this one would eliminate the need for _.bindClassUnderscoreFunctions(this); completely.

Overall should you consider using factory functions it may actually simplify the codebase and improve the code's portability as functions require no backwards transpilation to ES5 while at the same time improving composability by avoiding this and working only with plain objects instead of class instances.

No need to make everything a factory function but for something like the new Selectable(); helper it could be an improvement.

This is what the selectable.js helper would look like

import * as _ from './../lib/utils';

export default function Selectable(opt) {
    const selectable = {
        // Assign default values
        options: Object.assign({
            onchange: () => 0,
            className: ''
        }, opt),

        _ontap(evt) {
            const opt = selectable.options;

            opt.elements.forEach(e => e.classList.remove(opt.className));
            evt.target.classList.add(opt.className);

            opt.onchange();
        },

        destroy() {
            _.off(selectable.options.elements, 'click', selectable._ontap);
        }
    }

    _.on(selectable.options.elements, 'click', selectable._ontap);

    return selectable;
}

Note: changing this file in the codebase as it stands to the above code will not be breaking as calling new (function () {}) does not cause an error.

Perhaps read this article from Eric Elliott for more information on factory functions.

@simonwep
Copy link
Owner

Yeah, that's true... would simplify a lot. I personally prefer ES6 classes 'cause I use babel anyway and it will be transpiled to ES5 constructor functions. And they are more legible IMO. But you're basically right, I will take this in consideration / try it out.

@simonwep simonwep added the in consideration Feature, improvement or enhancement is in consideration label Jul 18, 2018
@SaulDoesCode
Copy link
Contributor Author

I agree factories are not as clean looking as classes, that's fair 😐.
Thanks for giving it a try though 👍.
But either way happy coding! 😃

@simonwep
Copy link
Owner

Hahaha, thank's 😄, I will have!
Also thank's for being so active, your improvements / suggestions are awesome! Helps a lot to improve this library!

@SaulDoesCode
Copy link
Contributor Author

SaulDoesCode commented Jul 18, 2018

class vs factory function: general structure comparison

this is just for interest's sake.
note: this is my take on the matter and could be very wrong indeed, tread carefully.

classes

class X {
   constructor (props) {
      Object.assign(this, {...X.defaults}, props)
   }
   
   method () {
     // do something
     console.log(`state: a: ${this.a}, b: ${this.b}, c: ${this.c}`)
   }

   //  ... more methods ...
}
// static defaults
X.defaults = {a: 1, b: 2, c: 3}

// USE:
const myX = new X({a: 45, b: 32});
myX.method()
// > state: a: 45, b: 32, c: 3

factory function

const X = (props = {}) => {
      // create object (equivalent to a class's 'this')
      const x = {
          ...X.defaults,
          ...props,

          method () {
              // do something
             console.log(`state: a: ${x.a}, b: ${x.b}, c: ${x.c}`)
          }

         //  ... more methods ...
      }
    
     // constructor area
    // ...nothing to do here...

   // return object as if it would be a class instance
   return x
}

// static defaults
X.defaults = {a: 1, b: 2, c: 3}

// USE:
const myX = X({a: 45, b: 32});
myX.method()
// > state: a: 45, b: 32, c: 3

now super condensed

const X = (props = {}) => (props = {
   ...X.defaults,
   ...props,
   method () { 
     console.log (`state: a: ${props.a} b: ${props.b} c: ${props.c}`)
   }
})
X.defaults = {a: 1, b: 2, c: 3}
moveable.js as a factory function
import * as _ from './../lib/utils';

export default function Moveable(opt) {
    const M = {
        // Assign default values
        options: Object.assign({
            lockX: false,
            lockY: false,
            onchange: () => 0
        }, opt),

        _tapstart(evt) {
            _.on(document, ['mouseup', 'touchend', 'touchcancel'], M._tapstop);
            _.on(document, ['mousemove', 'touchmove'], M._tapmove);

            // Trigger move
            M.trigger(evt);

            // Prevent default touch event
            evt.preventDefault();
        },

        _tapmove(evt) {
            const element = M.options.element;
            const b = M.wrapperRect;

            let x, y;
            if (evt) {
                const touch = evt && evt.touches && evt.touches[0];
                x = evt ? (touch || evt).clientX : 0;
                y = evt ? (touch || evt).clientY : 0;

                // Reset to bounds
                if (x < b.left) x = b.left;
                else if (x > b.left + b.width) x = b.left + b.width;
                if (y < b.top) y = b.top;
                else if (y > b.top + b.height) y = b.top + b.height;

                // Normalize
                x -= b.left;
                y -= b.top;
            } else if (M.cache) {
                x = M.cache.x;
                y = M.cache.y;
            } else {
                x = 0;
                y = 0;
            }

            if (!M.options.lockX)
                element.style.left = (x - element.offsetWidth / 2) + 'px';

            if (!M.options.lockY)
                element.style.top = (y - element.offsetHeight / 2) + 'px';

            M.cache = {x, y};
            M.options.onchange(x, y);
        },

        _tapstop() {
            _.off(document, ['mouseup', 'touchend', 'touchcancel'], M._tapstop);
            _.off(document, ['mousemove', 'touchmove'], M._tapmove);
        },

        trigger(evt) {
            M.wrapperRect = M.options.wrapper.getBoundingClientRect();
            M._tapmove(evt);
        },

        update(x = 0, y = 0) {
            M._tapmove(new MouseEvent('mousemove', {
                clientX: M.wrapperRect.left + x,
                clientY: M.wrapperRect.top + y
            }));
        },

        destroy() {
            _.off([M.options.wrapper, M.options.element], 'mousedown', M._tapstart);
            _.off([M.options.wrapper, M.options.element], 'touchstart', M._tapstart, {
                passive: false
            });
        }
    }

    M.wrapperRect = M.options.wrapper.getBoundingClientRect();

    _.on([M.options.wrapper, M.options.element], 'mousedown', M._tapstart);
    _.on([M.options.wrapper, M.options.element], 'touchstart', M._tapstart, {
        passive: false
    });

    return M;
}

@simonwep
Copy link
Owner

Reminds me of the Java-Factory-Pattern :D. Basically a very clean style, and can be better minified!

@simonwep
Copy link
Owner

Okay, (finally) I've borrowed it :)
It has proven itself to be very useful, was able to go down from 23,7 KB (24.303 bytes) to 22,6 KB (23.242 bytes). With some other changes even to 22,6 KB (23.177 bytes) 🎉 Altogether an awesome idea :D

@simonwep simonwep added improvement Something could be improved and removed in consideration Feature, improvement or enhancement is in consideration labels Aug 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something could be improved
Projects
None yet
Development

No branches or pull requests

2 participants