Need suggestions to improve the code for an open source web based drafting application

Question

My application. The repository and the file that needs review.

Here is the source code

$(function () {
  var draft = function (parsed, title) {
    var self = this;
    var wordCount = parsed.wordCount;
    self.date = new Date(parsed.time).toDateString();
    self.count = wordCount;
    self.title = title;
    self.plural = wordCount > 1;
    self.trueDate = new Date(parsed.time);
  };
  var viewModel = function (drafts) {
    var self = this;
    self.drafts = ko.observableArray(drafts);
    self.showEditor = ko.observable(true);
    self.showTitle = ko.observable(true);
    self.raw = ko.observable(true);
    self.deleteDraft = function (draft, event) {
      event.stopPropagation();
      removeDraft(draft.title);
      self.drafts.remove(draft);
    };
    self.showDrafts = function () {
      self.showEditor(false);
      self.showTitle(false);
      renderSavedDrafts();
      hideThis(["#previewContainer"]);
      showThis(["#drafts"]);
    };
    self.newDraft = function () {
      hideThis(["#previewContainer", "#drafts"]);
      self.showTitle(true);
      self.showEditor(true);
      editArea.focus();
      editArea.val('');
      $("#title").text('');
    };
    self.showPreview = function () {
      if (validateInputOnFousOut()) {
        setHtmlinPreviewPane(getMarkdownText());
        hideThis(['#plain']);
        self.showEditor(false);
        self.showTitle(true);
        showThis(["#rawHtml", '#previewContainer']);
        saveCurrentDraft();
        $("#saveStatus").fadeIn().show().delay(1000).fadeOut();
      }
    };
    self.hidePreview = function () {
      hideThis(["#previewContainer"]);
      self.showEditor(true);
      editArea.trigger('autosize');
      editArea.focus();
    };
    self.editDraft = function (draft) {
      var title = draft.title;
      var item = getDraftFromKey(title);
      var parsed = JSON.parse(item);
      hideThis(["#drafts"]);
      editArea.val(parsed.text).trigger('autosize');
      $("#title").text(title);
      $("#wordCount").text(parsed.wordCount);
      self.showEditor(true);
      self.showTitle(true);
    };
    self.rawHtml = function (data, event) {
      setRawHtml();
      event.stopPropagation();
      self.raw(false);
    }
    self.plain = function (data, event) {
      setPlain();
      event.stopPropagation();
      self.raw(true);
    }
  };

  function prepareInitialWorkSpace() {
    var editArea = $("#editArea");
    editArea.autosize();
    $("#title").focus();
    return editArea;
  }

  function hideThis(elements) {
    for (var i = 0; i < elements.length; i++) {
      $(elements[i]).hide();
    }
  }

  function showThis(elements) {
    for (var i = 0; i < elements.length; i++) {
      $(elements[i]).show();
    }
  }

  function getMarkdownText() {
    return $("#editArea").val();
  }

  function getWordCount(text) {
    return text.split(/\s+\b/).length;
  }

  function setHtmlinPreviewPane(markdownText) {
    $("#wordCount").text('words: ' + getWordCount(markdownText));
    $("#previewPane").html(markdown.toHTML(markdownText));
  }

  function setRawHtml() {
    var pane = $("#previewPane");
    pane.text(pane.html());
  }

  function setPlain() {
    var pane = $("#previewPane");
    pane.html(pane.text());
  }

  function getWordCountFromLabel(text) {
    return text.match(/\d+/)[0];
  }

  function validateInputOnFousOut() {
    var isTitleEmpty = $("#title").text().trim() === '';
    var isDraftEmpty = $("#editArea").val() === '';
    var hasTitileAndDraft = !isTitleEmpty && !isDraftEmpty;
    return hasTitileAndDraft;
  }
  var initializeDrafts = new viewModel();
  ko.applyBindings(initializeDrafts);

  function loadSavedDrafts() {
    return Object.keys(localStorage);
  }

  function sortedArray(data) {
    return data.sort(function (a, b) {
      a = new Date(a.trueDate);
      b = new Date(b.trueDate);
      return a < b ? -1 : a > b ? 1 : 0;
    }).reverse();
  }

  function buildData(keys) {
    var data = [];
    for (var i = 0; i < keys.length; i++) {
      var parsed = JSON.parse(localStorage[keys[i]]);
      var initializeDraft = new draft(parsed, keys[i]);
      data.push(initializeDraft);
    }
    return sortedArray(data);
  }

  function renderSavedDrafts() {
    var array = buildData(loadSavedDrafts());
    initializeDrafts.drafts(array);
  }

  function saveCurrentDraft() {
    var key = $("#title").text();
    var draft = {};
    draft["time"] = new Date();
    draft["text"] = getMarkdownText();
    draft["wordCount"] = getWordCountFromLabel($("#wordCount").text());
    localStorage.setItem(key, JSON.stringify(draft));
  }

  function getDraftFromKey(key) {
    return localStorage.getItem(key);
  }

  function removeDraft(key) {
    localStorage.removeItem(key);
  }
  var editArea = prepareInitialWorkSpace();
});

My main concern is that I have heavily mixed knockout and jquery. I need some tips to make my code less confusing and easier to understand. Any help will be appreciated.

Answer 1

I suggest you break down your application into concerns. Break them so that you separate your UI code, your IO code (read/write), your logic and your utilities. Then you could use a dependency system like Require to manage these separated code into modules.

Don't worry if you end up with multiple files. You can use their optimizer after you got everything done.

Also, you seem to use a few non-cross-brower-friendly APIs there (especially for IE), like Object.keys, localStorage and JSON. Be sure to check for safety and use polyfills if necessary if you want to support such functionality for older (cough, IE, cough) and buggy browsers.

As for your code, I have found a few weird parts that can be optimized.

• Organize your functions in the following manner:

  1. variable declarations
  2. function declarations
  3. all other stuff (in any order now)
     • function calls
     • assignment operations
     • etc.

  This is how the compiler sees your code. To avoid any weirdness or hard to find variables (like initializeDrafts and editArea) in your code, just follow the format.

• Prefer function declarations over function expressions when necessary. The difference between the two in terms of syntax is:

  function foo(){}        //declaration
  var foo = function(){}; //expression
  

  There is what you call "function and variable declaration hoisting" in JavaScript where function and variable declarations are pulled up in the scope which results in the order discussed previously.

  However, function expressions are assigment operations and are not hoisted. They exist where they are assigned. So unlike a function declaration which can be used before they are declared (since the compiler pulls it up), function expressions cannot be used before it was assigned.

  A misplaced function expression could lead to errors, while a misplaced function declaration is bad practice (IMO). Stick to the order, it's better.

• When selectors are often reused, it's best you store the selector string into a variable for easy modification. What if you changed the id of the element, you wouldn't want to edit tens of selectors (Although you could do a Replace All, but with considerable amount of risk).

  So from your viewModel, you can do:

  var viewModel = function (drafts) {
  
      var self = this;
      //cache selectors for easy modification
      var previewContainerSelector = '#previewContainer';
      var draftsSelector = '#drafts';
      ...
  
      self.showDrafts = function () {
        ...
        hideThis(previewContainerSelector);
        showThis(draftsSelector);
      };
      self.newDraft = function () {
        hideThis([previewContainerSelector, draftsSelector]);
        ...
      }
      ...
  

• When an element is static throughout the lifetime of your page, it's not good to "refetch" them from the DOM everytime. Since they are static, they won't change and thus you can just store them in a variable for reference.

  So in modification of the one above, you can also cache the element instead:

  var viewModel = function (drafts) {
  
      var self = this;
      //we can cache the jQuery objects of the elements instead
      var previewContainer = $('#previewContainer');
      var drafts = $('#drafts');
      ...
  
      self.showDrafts = function () {
        ...
        hideThis(previewContainer);
        showThis(drafts);
      };
      self.newDraft = function () {
        hideThis([previewContainer, drafts]);
        ...
      }
      ...
  

  You can even cache them outside the viewModel so it's accessible from the other functions, constructors and operations. That way, you fetch it only once from the DOM.

• The jQuery functions jQuery() and @() (I know, they are the same thing), can accept a variety of arguments. Try to harness their power instead of doing it manually.

  You can pass in an array of selectors

  function hideThis(elements) {
    var selectors = elements.join(',');
    $(selectors).hide();
  }
  
  hideThis(['#previewContainer','#drafts']);
  
  //you can even do:
  $(['#previewContainer','#drafts'].join(',')).hide();
  

  An array of DOM elements

  var previewContainer = document.getElementById('previewContainer');
  var drafts = document.getElementById('drafts');
  
  function hideThis(elements) {
    $(elements).hide();
  }
  
  hideThis([previewContainer,drafts]);
  

  An array of jQuery objects

  var previewContainer = $('#previewContainer');
  var drafts = $('#drafts');
  
  function hideThis(elements) {
    $.each(elements,function(){
      this.hide();
    });
  }
  
  hideThis([previewContainer,drafts]);
  

  You can also use the special arguments variable within each function which collects all arguments passed into the function into this variable. That way you don't need to construct an array of somethings. Just pass them in as normal arguments to the function, and access them via arguments like an array.