JavaScript anti-patterns

Let's talk about anti-patterns when writing frontend javascript. I've tried to avoid writing down the stuff that linters can tell you, and instead collect examples from the everyday code.

DOM

Starting with very basic and classic one. Conditions add unnecessary complexity in this case, use toggle. Rewrite

if (flag) {  
  node.addClass('someClass');
} else {
  node.removeClass('someClass');
}

as

node.toggleClass('someClass', flag);  

Avoiding DOM modifications is not only good for the performance, but forces more scalable code. Simple example: after doing an action on button click we need to hide the button.

Bad way to do that is to remove the element completely:

<div class="app">  
  <button class="button">Remove me</button>
</div>

<script>  
  $('.button').on('click', function() {
    $(this).remove();
  });
</script>  

One can use $.fn.hide instead, but it doesn't solve other issues.

Another way to do it, is to hide button by adding a class indicating that action is done to the root element:

<script>  
  var app = $('.app');

  $('.button').on('click', function() {
    app.addClass('app--action-done');
  });
</script>

<style>  
  .app--action-done .button {
    display: none;
  }
</style>  

In this and following examples, where the html/css/js code is mixed via script or style tags, I have different files in mind.

This solution gives more control over the state, where you can separate UI/design and programming. The logic "hide the button when action is done" is only in CSS now. Less code correction would be needed for UI changes with well-though states.

For CSS naming conventions you may use BEM.


Let's consider more complex example. We have a button, which sends a request to the server and as a result we need to show a message.

Straightforward code may look like this:

<div class="status"></div>

<script>  
$.post('/action').done(function(status) {
  switch (status) {
    case 'success':
      $('.status').html('Done');
    break;

    case 'timeout':
      $('.status').html('Try again');
    break;

    case 'error':
      $('.status').html('Error');
    break;
  }
});
</script>  

First of all, case conditions like that can be written cleaner

var responseText = {  
  success: 'Done',
  timeout: 'Timeout'
  error:   'Error'
};

$('.status').html(responseText[status]);

Still, there are couple of problems here. We have some text inside the code, that would be a trouble for the multi language site. However we can generate this object from the server and dump it in inside <script></script> on the page.

Instead, we can change our markup to

<div class="status">  
    <div class="message message--success">Done</div>
    <div class="message message--timeout">Try again</div>
    <div class="message message--error">Error</div>
</div>  

Here, we can use i18n capabilities of a server-side templating engine. Then, in JS:

$.post('/action').done(function(status) {
  $('.status').addClass('status--' + status);
});

And by adding CSS:

.status .message {
  display: none;
}

.status--success .message--success,
.status--timeout .message--timeout,
.status--error .message--error {
  display: block;
}

This gives us power to change the UI, for example to add red background or an animation for an error case without touching the JS.


When markup is stored inside JS it is hard to modify it. Instead of

// inside a loop
var html = '<li>' +  
  '<span>' + person.name + '</span>'
  '<span>' + person.age + '</span>' 
+ '</li>';

consider using a templating engine or if you are short on page size, use Doug Crockford's supplant:

<script type="text/template" id="person-tmpl">  
  <li><span>{name}</span><span>{age}</span></li>
</script>

<script>  
// cache it
var personTmpl = $('#person-tmpl').html().trim();

// use it
var html = personTmpl.supplant(person);  
</script>  

Same applies to string concatenation - interpolation is much easier to read.


When dealing with switchers and menus it is common to have a piece of code to set and remove active class.

Bad way to do it:

<ul class="list">  
  <li class="list__item">item 1</li>
  <!-- ... -->
  <li class="list__item">item N</li>
</ul>

$('.list__item').on('click', function() {
    $('.list__item').removeClass('list__item--active');
    $(this).addClass('list__item--active');
});

Instead of creating N listeners, it is better to use event delegation. Also class toggling could be written cleaner

var ACTIVE_CLASS = 'list__item--active';

$('.list').on('click', '.list__item', function() {
  $(this)
    .addClass(ACTIVE_CLASS)
    .siblings()
      .removeClass(ACTIVE_CLASS);
});

Obvious one, but still. Do not repeat yourself for the initialization and listeners.

$('.someElement').css({
  width: $(window).width() / 2
});

$(window).on('resize', function() {
  $('.someElement').css({
    width: $(window).width() / 2
  });
});

Reuse code through functions:

function resize() {  
  $('.someElement').css({
    width: $(window).width() / 2
  });
}

$(window).on('resize', resize);

// init
resize();  

<div class="el" data-counter="42"></div>

<script>  
var counter = $('.el').data('counter');

$('.el').attr('data-counter', counter + 1);
</script>  

It is better not to write to "data-" attributes, such behaviour is hard to debug. Use them as read-only and store the state somewhere inside the hash object.


Understand the difference between preventDefault and return false when handling events.

<div class="app">  
  <button class="btn">Click me</button>  
</div>

<script>  
$('.btn').on('click', function(event) {
  console.log('click 1', event);
  return false;
});

$('.app').on('click', '.btn', function(event) {
  console.log('click 2', event);
});
</script>  

Click 2 will not be printed, because you prevent bubbling with return false. This impairs code extensibility.


General JS

Instead of writing

if (obj.longPropertyName === 'a' ||  
    obj.longPropertyName === 'b' ||
    obj.longPropertyName === 'c' ||
    obj.longPropertyName === 'd' || /* ... */) {

  // do stuff
}

you can use object notation

if ({  
  a: 1, b: 1, c: 1, d: 1 /*, ... */
}[ obj.longPropertyName ]) {
  // do stuff
}

Use event emitter pattern for actions which are not directly connected to the module logic.

Say you want to add Google Analytics tracking to the app. Instead of inserting tracking code through the codebase, leave an event trigger and gather tracking code in one place:

// in handlers
App.Event.trigger('popup:closed');

// later
App.Event  
.on('popup:closed', function() {
  // track it to GA
})
.on('request:sent', function() {
  // track it to GA
});

As a pub-sub you can use jQuery: App.Event = $(window);.


Consider using constructor functions for the type conversions. This way the intent of operation is much clearer:

// instead of +stringNumber or parseInt(stringNumber, 10)
Number(stringNumber) 

// instead of ("" + i)
String(i)

// instead of !!someInt
Boolean(someInt)  

If you actively using promises in your app I suggest you checking great anti-pattern post about them.


When writing production code, please remember that in ten years there could be someone trying to figure out that hacky line you left in the code, so keeping it simple and reasonable commenting are probably the most important things you can do.

comments powered by Disqus