2
\$\begingroup\$

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);
        }
    }
};
\$\endgroup\$
4
  • \$\begingroup\$ This piece of code looks suspicious: $('.boxnotificationsusers').children().find('#boxsendnotifi'). Id values should be unique in the document, but this looks like you expect to fine several '#boxsendnotifi' items. \$\endgroup\$
    – jfriend00
    May 10 '15 at 17:41
  • \$\begingroup\$ It may be the HTML which I didn't write? right? \$\endgroup\$
    – Akar
    May 10 '15 at 17:43
  • \$\begingroup\$ @Akar - it doesn't matter if your wrote the HTML or not. There should not be multiple elements with the same ID. \$\endgroup\$
    – jfriend00
    May 10 '15 at 17:43
  • \$\begingroup\$ That's right. I should use class instead. \$\endgroup\$
    – Akar
    May 10 '15 at 17:44
3
\$\begingroup\$

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(...)
\$\endgroup\$
3
  • \$\begingroup\$ @Akar - though I'm happy you accepted my answer, in the future, you may want to wait hours or days before accepting an answer to see if others will submit other ideas. This problem would also lend itself to the module pattern with private variables and local functions. Since there is rarely a single right answer in codereview, it's beneficial to see several different ideas. \$\endgroup\$
    – jfriend00
    May 10 '15 at 18:09
  • \$\begingroup\$ Thanks, I'm not sure what's that pattern is called, The one that I'm using... \$\endgroup\$
    – Akar
    May 10 '15 at 18:27
  • \$\begingroup\$ method init looks like wrapper on bindEvents, I will remove it maybe \$\endgroup\$
    – Viktor
    May 13 '15 at 8:54
0
\$\begingroup\$

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);
        }
    }
};
\$\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.