jQuery Refactoring - DRY

Question

I have these two functions, inside of my main function. As you'll see the only difference between the two of them is in the middle with how they append/edit the html. I am thinking it would be nice to come up with two new functions, one that does the first half and the other that does the second half. I'm not sure if this is possible with jQuery, or even with JavaScript for that matter, as I don't know how to call these new functions inside of these functions, if that makes sense. Any help/guidance would be great!

Here is the first one

$('#save').click(function(){
var title = $('#title').val();
var tags = $('#tags').val();
var notes = $('#notes').val();
var myDate = new Date();
if (title.length < 1) {
  $('.title-warn').show();
}
if (tags.length < 1) {
  $('.tags-warn').show();
}
if (notes.length < 1) {
  $('.notes-warn').show();
}
if (title.length >= 1 && tags.length >= 1 && notes.length >= 1) {
  $allNotes.prepend('<li class="note"><div><h1>' + title + '</h1><div class="date">      <h2>'+ myDate.toDateString() +'</h2><span class="btn btn-edit">Edit</span></div><h3>' + tags + '</h3><p>' + notes + '</p></div></li>');
  $allNotes.show();
  $newNote.hide();
  $('.title-warn').hide();
  $('.tags-warn').hide();
  $('.notes-warn').hide();
}
$('#title').val('');
$('#tags').val('');
$('#notes').val('');
$('#search').prop('disabled', false);
$('#search').attr("placeholder", "Search by title, tags, date, or even words/sentences in notes");
$('.btn-search').prop('disabled', false);
});

And now the second one

$('#edit').click(function(){
var title = $('#edit-title').val();
var tags = $('#edit-tags').val();
var notes = $('#edit-notes').val();
var myDate = new Date();
if (title.length < 1) {
  $('.title-warn').show();
}
if (tags.length < 1) {
  $('.tags-warn').show();
}
if (notes.length < 1) {
  $('.notes-warn').show();
}
if (title.length >= 1 && tags.length >= 1 && notes.length >= 1) {
  $('.edited-note').html('<div><h1>' + title + '</h1><div class="date">      <h2>'+ myDate.toDateString() +'</h2><span class="btn btn-edit">Edit</span></div><h3>' + tags + '</h3><p>' + notes + '</p></div>');
  $('.allnotes').show();
  $('.edit-note').hide();
  $('.title-warn').hide();
  $('.tags-warn').hide();
  $('.notes-warn').hide();
}
$('#title').val('');
$('#tags').val('');
$('#notes').val('');
$('#search').prop('disabled', false);
$('#search').attr("placeholder", "Search by title, tags, date, or even words/sentences in notes");
$('.btn-search').prop('disabled', false);
$('.edited-note').removeClass('edited-note');
});

And of course if anyone has any suggestions on any other aspects of my code I am up for criticism!

Answer 1

You answered your own question! "As you'll see the only difference between the two of them is in the middle with how they append/edit the html." An initial pretty naive and mechanical attempt attempt at refactoring might look like something like this, just pulling out all the common code into shared functions:

function preHandler(title, tags, notes) {
    if (title.length < 1) {
        $('.title-warn').show();
    }
    if (tags.length < 1) {
        $('.tags-warn').show();
    }
    if (notes.length < 1) {
        $('.notes-warn').show();
    }

    return title.length >= 1 && tags.length >= 1 && notes.length >= 1;

}


function commonPost () {

    $('#title').val('');
    $('#tags').val('');
    $('#notes').val('');
    $('#search').prop('disabled', false);
    $('#search').attr("placeholder", "Search by title, tags, date, or even words/sentences in notes");
    $('.btn-search').prop('disabled', false);


}



$('#save').click(function(){
    var title = $('#title').val();
    var tags = $('#tags').val();
    var notes = $('#notes').val();
    var myDate = new Date();
    if  (preHandler(title, tags, notes)) {

        $allNotes.prepend('<li class="note"><div><h1>' + title + '</h1><div class="date">      <h2>'+ myDate.toDateString() +'</h2><span class="btn btn-edit">Edit</span></div><h3>' + tags + '</h3><p>' + notes + '</p></div></li>');
        $allNotes.show();
        $newNote.hide();
        $('.title-warn').hide();
        $('.tags-warn').hide();
        $('.notes-warn').hide();

    }
    commonPost();

});


$('#edit').click(function() {
    var title = $('#edit-title').val();
    var tags = $('#edit-tags').val();
    var notes = $('#edit-notes').val();
    var myDate = new Date();

    if  (preHandler(title, tags, notes)) {

        $('.edited-note').html('<div><h1>' + title + '</h1><div class="date">      <h2>'+ myDate.toDateString() +'</h2><span class="btn btn-edit">Edit</span></div><h3>' + tags + '</h3><p>' + notes + '</p></div>');
        $('.allnotes').show();
        $('.edit-note').hide();
        $('.title-warn').hide();
        $('.tags-warn').hide();
        $('.notes-warn').hide();

    }

    commonPost();
    $('.edited-note').removeClass('edited-note');

});

It doesn't really win you that much when there are only two scenarios. But with more than two doing this sort of refactoring starts to reap rewards.

This first attempt could be refined (a lot). Perhaps nice folks will post. But this would be a good first attempt.

Answer 2

Generally I like creating some sort of a controller class which does all the UI work for each page in my application. This makes testing a whole lot easier because you can invoke your target methods in a very easy manner.

I also like prefer hiding my selectors behind easy to read variables, so if I were to read this file in a few weeks/months from now, I can quickly bring myself to speed by reading what the high-level logic is intending to do. (FYI in my answer below I haven't done this. I actually don't know what editTitle is supposed to represent. But here are few examples of how I would rename it as: TaskTitle, BlogTitle, etc.)

These are all guidelines really, but I wished I had always written my code like this. I would recommend you to take out some time and just read about javascript design patterns, programming idioms, etc.

var myApp = new app();

$(function () {
    myApp.init();
});

var app = function () {
    var editTitle = "#edit-title";
    var editTitleWarning = ".title-warn";

    var editTags = "#edit-tags";
    var editTagsWarning = ".tags-warn";

    var editNotes = "#edit-notes";
    var editNotesWarning = ".notes-warn";

    var saveButton = "#save";
    var editButton = "#edit";

    var updateUI = function (args) {
        var isSave = args.data.isSave;
        var title = $(editTitle).val();
        var tags = $(editTags).val();
        var notes = $(editNotes).val();
        var myDate = new Date();

        // (suggestion) add comment here 
        if (title.length < 1) {
            $(editTitleWarning).show();
        }

        // (suggestion) add comment here 
        if (tags.length < 1) {
            $(editTagsWarning).show();
        }

        // (suggestion) add comment here 
        if (notes.length < 1) {
            $(editNotesWarning).show();
        }

        if (isSave) {
            // add save append code here
        } else {
            // add edit html code here
        }

        // add remaining code here
    };

    this.init = function () {
        $("body")
            .on("click", saveButton, { isSave = true }, updateUI)
            .on("click", editButton, { isSave = false }, updateUI);
    };
}

Answer 3

Talking only about making the code DRY (versus about architecturing it properly in general), I'd probably rewrite the code as:

var handler = function(prefix, htmlHandler, removeEditedNoteCls) {
    prefix = '#' + (prefix === false ? '' : (prefix + '-'));
    var list = ['title', 'tags', 'notes'],
      i = 0,
      h = {
        date: new Date
      },
      act = true;
    for (; i < list.length; i++) {
      h[list[i]] = $(prefix + list[i]).val();
      if (h[list[i]].length < 1) {
        $('.' + list[i] + '-warn').show();
        act = false;
      }
    }
    if (act) {
      htmlHandler.call(this, h);
      for (i = 0; i < list.length; i++) {
        $('.' + list[i] + '-warn').hide();
      }
    }
    for (i = 0; i < list.length; i++) {
      $('#' + list[i]).val('');
    }
    $('#search').prop('disabled', false);
    $('#search').attr("placeholder", "Search by title, tags, date, or even words/sentences in notes");
    $('.btn-search').prop('disabled', false);
    if (removeEditedNoteCls) {
      $('.edited-note').removeClass('edited-note');
    }
  },
  prepend = function(h) {
    $allNotes.prepend('<li class="note"><div><h1>' + h.title + '</h1><div class="date">      <h2>' + h.date.toDateString() + '</h2><span class="btn btn-edit">Edit</span></div><h3>' + h.tags + '</h3><p>' + h.notes + '</p></div></li>');
    $allNotes.show();
    $newNote.hide();
  },
  replace = function(h) {
    $('.edited-note').html('<div><h1>' + h.title + '</h1><div class="date">      <h2>' + h.date.toDateString() + '</h2><span class="btn btn-edit">Edit</span></div><h3>' + h.tags + '</h3><p>' + h.notes + '</p></div>');
    $('.allnotes').show();
    $('.edit-note').hide();
  };
$('#save').click(function() {
  handler('', prepend);
});
$('#edit').click(function() {
  handler('edit', replace, true);
});

Basically, you:

1. Don't repeat var per earch variable declaration. Use commas to separate declarations under the same var. Although not a big deal with DRY but it makes code shorter and nicer.
2. Identify repeating/similar stuff and squash it into either:
   • Loops over arrays which contain the differences only. In your case it will be the array of IDs ['title', 'tags', 'notes']; OR
   • Methods accepting the differences as arguments. In your case both "edit" and "save" handlers are pretty similar, which should be the first signal to you to wrap them into a named method (handler in my example).