2
\$\begingroup\$

I wrote an implementation of observer pattern as part of my online battle Chess game:

const observers = Symbol();

class Observer{

    constructor(){

        if(new.target === Observer){

            throw new Error('Cannot create new Observer object. Observer is supposed to be inherited only.');
        }

        /**
         * @private
         * @type {Set}
         */
        this[observers] = new Set();
    }

    on(observer, event, callback){

        this.getObservers().add({

            observer,
            event,
            callback
        });
    }

    off(observer, event){

        const observers = this.getObservers();
        const observerEntries = this.getObservers().values();

        for(let entry of observerEntries){

            if(entry.observer === observer && entry.event === event){

                observers.delete(entry);
            }
        }
    }

    notify(event, data = {}){

        const observerEntries = this.getObservers().values();

        for(let entry of observerEntries){

            if(entry.event === event){

                entry.callback.call(entry.observer, data);
            }
        }
    }

    getObservers(){

        return this[observers];
    }
}

module.exports = Observer;

The source code for the full game is in a GitHub repository. This is an early version, without many features implemented and possibly with many bugs. I have a humble request to review my code. I'm aware of several things (backend Node code is very messy). The client-side is written in MVC and Observer patterns. I'm looking for feedback (both positive and negative) and advice/guidelines.

\$\endgroup\$
  • \$\begingroup\$ Question edited, though in case of rather larger projects I don't see any point in posting single short examples of code. \$\endgroup\$ – Furman Feb 5 '18 at 18:22
  • \$\begingroup\$ This is outside the scope of this Code Review question, but your login page code crashes here because input[type=text is malformed. \$\endgroup\$ – 200_success Feb 5 '18 at 20:30
1
\$\begingroup\$

The name of the Observer class is confusing: an Observer contains observers? I think that the class would make more sense if you renamed it Observable.

For the .on() and .off() methods, I think that the event parameter should come first. That would mimic the natural English expression: "on event tell observer to do action". It's also how it's done in other frameworks like jQuery.

I'm mildly annoyed by the excessive vertical spacing of the code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.