4
\$\begingroup\$

I would like to improve my code and organization of JS files so I'm wondering if you guys could tell me what I could do better. I've included a small but typical example of how I often use components.

With this component I'm able to show different kind of notifications to the user, which then disappear after a set amount of time.

I use these components by adding this in the appropriate location: new notification('warning', $('.sourcecode-warning').text(), 20000);. Rip away!

function notification(notificationType, message, delay) {
var _this = this;
this.notificationType = notificationType;
this.className = 'notification';
this.message = message;
this.delay = delay || 3000;
this.removeTimeOut;
this.disappearTimeOut;

this.init = function () {
    $('body').append('<div class=" ' + this.className + ' ' + _this.notificationType + '"><p>' + _this.message + '</p><i class="fa fa-times fa-lg close-notification"></i></div>');
    _this.setEvents();

    _this.removeTimeOut = setTimeout(function () {
        _this.startDisappearing();
    }, _this.delay);
};

this.setEvents = function () {
    $('.close-notification').bind('click', function () {
        _this.removeSelf();
    });
};

this.startDisappearing = function () {
    $('.' + _this.className).addClass('disappearing');

    _this.disappearTimeOut = setTimeout(function () {
        _this.removeSelf();
    }, 1200);
};

this.removeSelf = function () {
    clearTimeout(_this.removeTimeOut);
    clearTimeout(_this.disappearTimeOut);

    $('.' + _this.className).remove();
};

if ($('.' + _this.className).length === 0) _this.init();
};
\$\endgroup\$
3
\$\begingroup\$

I think you could improve a lot. :)

I don't think you need an init function, it just add stuff and the code is difficult to follow and understand.

The main function is yet your initialization.

Even the other functions are a big overhead and make the code complicated.

I put inline comment to explain the code.

function notification(notificationType, message, delay) {
  // you don't need to attach things to this, just use variables.
  const classNameSelector = 'notification';
  const disappearingClass = 'disappei'
  if ($('.'+classNameSelector).length === 0)
    return; // if component is still there you want to avoid recreate it, but
            // so don't need to load all the code, just exit at the beginning.

  // what about place in a template or in a hidden div?
  const _notification = $('<div class=" ' + classNameSelector + ' ' +
    notificationType + '"><p>' + message +
    '</p><i class="fa fa-times fa-lg close-notification"></i></div>');

  $(body).append(_notification);

  // here I'm a bit confuse, you have a delay, but you set an arbitrary number
  // to remove the notification box: 1200
  // So I changed it in a way that works for me:
  const startDiappearing = delay / 3
  const disappearingAll = delay - startDiappearing

  // the first timeout is startDisappearing
  let removeTimeout;
  const disappearingTimeOut = setTimeout(function() {
    _notification.addClass('disappearing');
    removeTimeout = setTimeout(function () {
        _notification.remove();
    }, disappearingAll);
  }, startDiappearing);

  // Last but not last the bind to the click event
  _notification.on('click', function() {
    clearTimeout(disappearingTimeOut);
    clearTimeout(removeTimeout);

    _notification.remove();
  });
}

I know you feel there is some duplication of remove() but is just a lib call and just one.

The purpose here is made something simple and short as effectively simple and short, avoiding overengineering some part of code.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply! Some good tips in there. I was wondering about your complete absence of functions though. I find them very descriptive and thus helping someone understand what is going on in the code. Especially for the bigger components as this one is very small. And why would you save the HTML string in a hidden div? I realize it makes it somewhat easier to manipulate but at the same time it pollutes the HTML as this notificiation might not even be used. Also there's a loss in speed due to having to add stuff into nodes instead of constructing a string and then adding it to the HTML \$\endgroup\$ – Jantje Sep 13 '17 at 12:40
  • \$\begingroup\$ @Jantje it is only an hint, if the string to build the HTML or the code start to became huge, it is better to use a template of some sort. So the HTML is much readable, and the code too. In this case, I agree with you, it's fine just put a string there. \$\endgroup\$ – Mario Santini Sep 13 '17 at 14:09
0
\$\begingroup\$

Function calls in callbacks

Instead of making a closure/lambda function just to call a function, the name of the function can be used instead. For example, the following block:

$('.close-notification').bind('click', function () {
    _this.removeSelf();
});

Could be simplified to:

$('.close-notification').bind('click', _this.removeSelf);

Bear in mind that the arguments passed to the click handler (e.g. the clickEvent) would be passed to the removeSelf method but those should be ignored.

Also, .bind() is deprecated in version 3; .click() (or other event binding helpers) can be used instead:

$('.close-notification').click(_this.removeSelf); 

Single element with class notification

Because the code won't add a notification element if there are already elements with that class name notification, the code could instead add a single hidden element when the DOM is ready, show it when the notification class is instantiated, and then hide it when necessary. That way the DOM wouldn't be manipulated as much. For more information, check out Stop writing slow Javascript. I know it bashes jQuery a bit in the beginning but later it has some useful information that I wish I had read years ago.

Removing _this

One could use Function.prototype.bind() (not to be confused with the jQuery method .bind()) to bind functions to the context of this, which would eliminate the need for the extra variable _this. See an example below, also utilizing the function names instead of calls in simple closures.

function notification(notificationType, message, delay) {
    //var _this = this;
    this.notificationType = notificationType;
    this.className = 'notification';
    this.message = message;
    this.delay = delay || 3000;
    this.removeTimeOut;
    this.disappearTimeOut;

    this.init = function() {
        $('body').append('<div class=" ' + this.className + ' ' + this.notificationType + '"><p>' + this.message + '</p><i class="fa fa-times fa-lg close-notification"></i></div>');
        this.setEvents();  
      
        this.removeTimeOut = setTimeout(this.startDisappearing.bind(this), this.delay);    
    };

    this.setEvents = function() {
        $('.close-notification').click(this.removeSelf);
    };

    this.startDisappearing = function() {
        $('.' + this.className).addClass('disappearing');
        this.disappearTimeOut = setTimeout(this.removeSelf.bind(this), 1200);
    };

    this.removeSelf = function() {
    console.log('args',arguments, this.className);
        clearTimeout(this.removeTimeOut);
        clearTimeout(this.disappearTimeOut);

        $('.' + this.className).remove();
    };
    
    if ($('.' + this.className).length === 0) this.init();
};
$(function() {
    $('#add').click(function() {
            new notification('warning', $('.sourcecode-warning').text(), 4000);
    });
});
.notification {
  border: 2px solid #0F0;
  background: #0e0;
  color: #000;
  padding: 8px;
  position: relative;
}
.notification .close-notification {
  
  position: absolute;
  right: 0;
  top: 0;
}
.label {
  font-style: italic;
}
<link href="//netdna.bootstrapcdn.com/font-awesome/4.0.3/css/font-awesome.min.css" rel="stylesheet"/>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<button id="add">Add notifiation</button>
  <div class="label">sourcecode-warning:</div>
  <div class="sourcecode-warning">
    Send us a message
  </div>

jQuery Element creation

One could utilize jQuery's helper function to create elements (see example below), or utilize a template engine.

var container = $('<div>',{"class": this.className + ' ' + this.notificationType});
var paragraph = $('<p>').append(this.message);
var close = $('<i>',{"class": 'fa fa-times fa-lg close-notification'});
container.append(paragraph, close);
$('body').append(container);
\$\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.