Should I use module pattern for this notification controller?

Question

I have the following code, I use this pattern for the entire JavaScript code. I'm trying to re-factor my code again. Should I use the Module pattern? Revealing Module Pattern to be specific?

What's happening is, somewhere in my code I say Dnianas.Notification.init().

First I bind the events, and I will handle them one by one.

Is there a better way to do this?

Also I notice that I don't use the var keyword. because they depend on each other.

Dnianas.Notification = {
    init: function () {
        this.bindEvents();
        this.cache();
    },

    bindEvents: function () {
        $(document).on('click', '#opennotifii', this.markAsRead);
    },

    cache: function () {
        $notification = $('#opennotifii');
    },

    countNotifications: function () {
        var $notifications = $('.boxnotificationsusers').children().find('#boxsendnotifi');
        ids = [];

        // Add unread notifications to the ids array.
        $notifications.each(function () {
            if ($(this).data('read') === 0) {
                ids.push($(this).data('id'));
            }
        });

        return ids;
    },

    markAsRead: function () {
        self = Dnianas.Notification;
        ids = self.countNotifications();

        if (ids.length > 0) {
            var request = $.ajax({
                url: '/notifications/read',
                data: {
                    notifications: ids,
                    _token: token,
                }
            });

            request.done(function (data) {
                self.renderNotificationCount(data);
            });
        }
    },

    renderNotificationCount: function (data) {
        if (data.seen) {
            $notification.find('.not_nu1').fadeOut(200);
        }
    }
};

Answer 1

Here's a modified version that uses the original singleton declaration.

Description of changes:

1. Remove all undeclared and thus implicitly global variables.
2. Remove caching of jQuery selector object (don't see any need for it).
3. Remove several intermediate variables that are only used once
4. Add .bind() to event handler so it can use this directly
5. Change from .done() to .then() to code more closely to the promise standard.

Modified code:

Dnianas.Notification = {
    init: function () {
        this.bindEvents();
    },

    bindEvents: function () {
        $(document).on('click', '#opennotifii', this.markAsRead.bind(this));
    },

    countNotifications: function () {
        var ids = [];

        // Add unread notifications to the ids array.
        $('.boxnotificationsusers').children().find('#boxsendnotifi').each(function () {
            if ($(this).data('read') === 0) {
                ids.push($(this).data('id'));
            }
        });
        return ids;
    },

    markAsRead: function () {
        var self = this;
        var ids = this.countNotifications();

        if (ids.length > 0) {
            $.ajax({
                url: '/notifications/read',
                data: {
                    notifications: ids,
                    _token: token,
                }
            }).then(function (data) {
                self.renderNotificationCount(data);
            });
        }
    },

    renderNotificationCount: function (data) {
        if (data.seen) {
            $('#opennotifii .not_nu1').fadeOut(200);
        }
    }
};

---

Also, this piece of code looks suspicious:

$('.boxnotificationsusers').children().find('#boxsendnotifi').each(...)

because there should only ever be one '#boxsendnotifi' item in the entire document so if that was the case, you could just do this:

$('#boxsendnotifi').each(...)

or perhaps:

$('.boxnotificationsusers #boxsendnotifi').each(...)

if you want to limit what you find to a particular scope.

Or, perhaps you should just be using a class name if there are potentially multiple matches:

$('.boxnotificationsusers .boxsendnotifi').each(...)

Answer 2

For me your code looks very pretty.

Sometimes I write in this manner

Dnianas.Notification = {
    elements: {
        opennotifiiSel: '#opennotifii', //also rename selectors to camelCase, for example
        boxsendnotifSel: '#boxsendnotif',
        boxnotificationsusersSel: '.boxnotificationsusers'
    },

    constants: {
        NOTIFICATION_FADE_OUT: 200
    },

    URL: {
        markAsRead: '/notifications/read',
    },

    init: function() {
        this.bindEvents();
    },

    bindEvents: function() {
        $(document).on('click', this.elements.opennotifiiSel, this.markAsRead.bind(this));
    },

    countNotifications: function() {
        var ids = [];

        // Add unread notifications to the ids array.
        $(this.elements.boxnotificationsusersSel)
            .children() //why we use children() before find()?
            .find(this.elements.boxsendnotifSel)
            .each(function collectboxsendnotifIds() {
                if ($(this).data('read') === 0) {
                    ids.push($(this).data('id'));
                }
            });
        return ids;
    },

    markAsRead: function() {
        var self = this;
        var ids = this.countNotifications();

        if (ids.length > 0) {
            $.ajax({
                url: self.URL.markAsRead,
                data: {
                    notifications: ids,
                    _token: token,
                }
            }).then(function(data) {
                self.renderNotificationCount(data);
            });
        }
    },

    renderNotificationCount: function(data) {
        if (data.seen) {
            $(this.elements.opennotifiiSel + ' .not_nu1').fadeOut(this.constants.NOTIFICATION_FADE_OUT);
        }
    }
};