4
\$\begingroup\$

Is the following a good way to use the revealing module pattern?

$(document).ready(function() {

  var wordsCounter = (function() {

    function init() {
      bindEvents();
    }

    // cache dom
    var $textArea = $('#textArea');
    var $words = $('#words'); //a span
    var $chars = $('#chars'); //a span

    function bindEvents() {
      $textArea.on('keyup', updateWordsCount);
    }

    // $textArea keyup callback function
    function updateWordsCount() {
      var text =  $textArea.val();
      var numOfChars = text.length;
      var numOfWords = countWords(text);
      $chars.html(numOfChars);
      $words.html(numOfWords);
    }

    // counts words for a given string
    function countWords(str){
      var cnt = 0;
      var flag = true;
      for(var i = 0; i < str.length; i++){
        if(str[i] !== " " && flag){
          flag = false;
          cnt++; 
        }else if(str[i] === " "){
          flag = true;
        }
      }
      return cnt;
    };

    return {
      init: init
    }

  }());

  wordsCounter.init();

});

What are other good ways I can structure this code?

Also if i'm working on a larger project, should it all be in a one IIFE like this or should I divide it into more than one IIFE each one doing related things?

\$\endgroup\$
  • \$\begingroup\$ how large is the larger project? if it's significantly larger than a couple of files I'd recommend using a module bundler like browserify, webpack or amd instead of the revealing module pattern \$\endgroup\$ – Dan Pantry Sep 10 '16 at 12:51
2
\$\begingroup\$
function bindEvents() {
  $textArea.on('keyup', updateWordsCount);
}

// $textArea keyup callback function
function updateWordsCount() {
  var text =  $textArea.val();
  var numOfChars = text.length;
  var numOfWords = countWords(text);
  $chars.html(numOfChars);
  $words.html(numOfWords);
}

The handler is unnecessarily split from the code that binds it. It's not often you see an event handler split off from it's binding. In most cases, the handler code is very specific to the binding, it cannot be reused and therefore makes no sense splitting off. Inline the handler instead.

var $textArea = $('#textArea');
var $words = $('#words'); //a span
var $chars = $('#chars'); //a span

I would discourage the use of $ indicating that a variable is a jQuery instance. Whether it's a jQuery instance, or some other constructors instance, a variable is a variable regardless of what it's bound to.

Another problem is that this piece of code isn't reusable. The selector uses an ID. IDs are supposed to be unique. That means this code can only bind to 3 unique elements, and no more. If you want to make this word counter reusable, use a class.

var flag = true;

flag is a poorly named variable. It's not telling me what it's supposed to be, what it is for, and why it is there.

// counts words for a given string
function countWords(str){
  var cnt = 0;
  var flag = true;
  for(var i = 0; i < str.length; i++){
    if(str[i] !== " " && flag){
      flag = false;
      cnt++; 
    }else if(str[i] === " "){
      flag = true;
    }
  }
  return cnt;
};

I think this is overcomplicated. A simple word count can be done by exploding the string into array by spaces, and getting the length of the resulting array. return str.split(' ').length what you are looking for.


For this simple task, a jQuery "plugin" would do. I would expect the plugin to accept an element containing your input and output elements, and a set of options asking for which elements are the input and output inside that group.

jQuery.fn.wordCounter = function(options){

  // For each found group
  return this.each(function(){

    // Look for the input and output under that group
    var input = $(options.input, this);
    var output = $(options.output, this);

    // Counter event and handler
    input.on('keyup', function(){
      output.text(input.val().split(' ').filter(text => text !== '').length);
    });

  });
};

$('.input-group').wordCounter({
  input: '.input-group__input',
  output: '.input-group__count',
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="input-group">
  <input class="input-group__input">
  <span class="input-group__count">
</div>

In larger scale projects, consider using a small framework. vue.js, Ractive.js and riot.js are good candidates for quick-and-easy UI-related operations.

\$\endgroup\$
  • \$\begingroup\$ Nice review, but something puzzles me: why do you would discourage the use of $ indicating that a variable is a jQuery instance? I totally agree that a variable is a variable regardless of what it's bound to but where is the bad thing in using such $s? And in the other hand, in my mind it's a useful mean to improve readability, especially when it allows to consistently designate variables that are mutually "parents" one vs the other, like $something bound to a DOM element vs something bound to a data the former one may contain or receive. \$\endgroup\$ – cFreed Sep 10 '16 at 20:01
  • \$\begingroup\$ @cFreed That doesn't really help at all with readability and only causes confusion and visual clutter. If prefixing $ to jQuery instances is your concept of readability, we should all prefix lodash and underscore chains with _, or prefix mori immutable structures with m, or promises with a p. But nobody really does that. A jQuery instance is as good as any other constructor's instance assigned to a variable. jQuery just happens to use the $ as a handy globa. FWIW, Mootools and prototype.js also use the $ global and both libraries are similar to jQuery. \$\endgroup\$ – Joseph Sep 10 '16 at 22:14
  • \$\begingroup\$ Talking about _, m, or p, sure that nobody really does that (and I'd not too, incidentally because I can't find any readability benefit when prefixing with a simple letter, but mainly because it wouldn't cover an interesting distinction like the "DOM vs not-DOM" in the case of jQuery). But you can't argue the same about $ with jQuery: AFAIK it tends to be widely used. Regarding the similar use of $ by other libraries, I agree but it simply means you may use the same convention when using them. Anyway I don't want to "fight" about that: I think it's rather matter of taste... \$\endgroup\$ – cFreed Sep 11 '16 at 0:55

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.