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);
 }
 }
};

• jfriend00: 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.
• Akar: It may be the HTML which I didn't write? right?
• jfriend00: @Akar - it doesn't matter if your wrote the HTML or not. There should not be multiple elements with the same ID.
• Akar: That's right. I should use class instead.

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(...)

• jfriend00: @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.
• Akar: Thanks, I'm not sure what's that pattern is called, The one that I'm using...
• Viktor: method init looks like wrapper on bindEvents, I will remove it maybe

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);
 }
 }
};