How to enhance the readability of JavaScript part?

Question

There is no doubt that Knockout.js is a very useful tool, which will save you from a lot of JavaScript (jQuery) binding hassle, which will reduce your team's bug ratio concerning this part.

But unfortunately, its JavaScript part gets ugly sooner than you can imagine. The resulting unreadable code is killing me and my team.

Here you can find a simple example from the official website.

This is an official example:

var savedLists = [
{ name: "Celebrities", userNames: ['JohnCleese', 'MCHammer', 'StephenFry', 'algore', 'StevenSanderson']},
{ name: "Microsoft people", userNames: ['BillGates', 'shanselman', 'ScottGu']},
{ name: "Tech pundits", userNames: ['Scobleizer', 'LeoLaporte', 'techcrunch', 'BoingBoing', 'timoreilly', 'codinghorror']}
];

var TwitterListModel = function(lists, selectedList) {
this.savedLists = ko.observableArray(lists);
this.editingList = {
    name: ko.observable(selectedList),
    userNames: ko.observableArray()
};
this.userNameToAdd = ko.observable("");
this.currentTweets = ko.observableArray([])

this.findSavedList = function(name) {
    var lists = this.savedLists();
    return ko.utils.arrayFirst(lists, function(list) {
        return list.name === name;
    });
};

this.addUser = function() {
    if (this.userNameToAdd() && this.userNameToAddIsValid()) {
        this.editingList.userNames.push(this.userNameToAdd());
        this.userNameToAdd("");
    }
};

this.removeUser = function(userName) { 
    this.editingList.userNames.remove(userName) 
}.bind(this);

this.saveChanges = function() {
    var saveAs = prompt("Save as", this.editingList.name());
    if (saveAs) {
        var dataToSave = this.editingList.userNames().slice(0);
        var existingSavedList = this.findSavedList(saveAs);
        if (existingSavedList) existingSavedList.userNames = dataToSave; // Overwrite existing list
        else this.savedLists.push({
            name: saveAs,
            userNames: dataToSave
        }); // Add new list
        this.editingList.name(saveAs);
    }
};

this.deleteList = function() {
    var nameToDelete = this.editingList.name();
    var savedListsExceptOneToDelete = $.grep(this.savedLists(), function(list) {
        return list.name != nameToDelete
    });
    this.editingList.name(savedListsExceptOneToDelete.length == 0 ? null : savedListsExceptOneToDelete[0].name);
    this.savedLists(savedListsExceptOneToDelete);
};

ko.computed(function() {
    // Observe viewModel.editingList.name(), so when it changes (i.e., user selects a different list) we know to copy the saved list into the editing list
    var savedList = this.findSavedList(this.editingList.name());
    if (savedList) {
        var userNamesCopy = savedList.userNames.slice(0);
        this.editingList.userNames(userNamesCopy);
    } else {
        this.editingList.userNames([]);
    }
}, this);

this.hasUnsavedChanges = ko.computed(function() {
    if (!this.editingList.name()) {
        return this.editingList.userNames().length > 0;
    }
    var savedData = this.findSavedList(this.editingList.name()).userNames;
    var editingData = this.editingList.userNames();
    return savedData.join("|") != editingData.join("|");
}, this);

this.userNameToAddIsValid = ko.computed(function() {
    return (this.userNameToAdd() == "") || (this.userNameToAdd().match(/^\s*[a-zA-Z0-9_]{1,15}\s*$/) != null);
}, this);

this.canAddUserName = ko.computed(function() {
    return this.userNameToAddIsValid() && this.userNameToAdd() != "";
}, this);

// The active user tweets are (asynchronously) computed from editingList.userNames
ko.computed(function() {
    twitterApi.getTweetsForUsers(this.editingList.userNames(), this.currentTweets);
}, this);
};

ko.applyBindings(new TwitterListModel(savedLists, "Tech pundits"));

// Using jQuery for Ajax loading indicator - nothing to do with Knockout
$(".loadingIndicator").ajaxStart(function() {
$(this).fadeIn();
}).ajaxComplete(function() {
$(this).fadeOut();
});

And of course, we have in our code base a lot more worse examples than that. Really bad ones, especially with a more complicated business.

So, do anyone know how to make Knockout.js's code more elegant and readable?

Answer 1

Quick solution, and not specific to knockout: I'd declare a variable up high in your constructor:

var instance = this;

and replace "this" with "instance" later in the class. You can use a variable name of your choosing, of course, but please bear with me.

My rationale is this thing I experience, as a senior developer, called "code fatigue". Simply stated, it's which "this" is "this"? I do much more c#, where (a) in-scope properties of the current class don't have to be prefixed with "this" and (b) best practice is one class per file -- which can be done in JS, but has to be planned out because of dependency issues.

So, my train of thought is that one of your devs is tasked to do a quick fix on something buggy. Let's He (she) is directed to look at a single function somewhere in a monster Javascript file. Let's say it's a junior developer, to add to the foray. And those "this" references are everywhere. It's INSTANTLY confusing to trace back up to the origin of "this", most especially if the code isn't formatted for viewing. But if you abstract the scope-level "this" into a neat, uniquely NAMED variable, it's a cleaner point of reference and takes the guesswork out.

Just a thought.

Answer 2

The sample code from knockout probably just wants to show what can be done, not how it ought to be done. ( You messed up indenting btw ).

If this were my code, I would have 3 or 4 models/objects:

• Groups
  
  • get ( aka findSavedList )
  • save ( aka saveChanges )
  • remove ( aka deleteList )
  • isDirty ( aka hasUnsavedChanges ) -> You might consider a Group model?
• UserList
  • add ( aka addUser )
  • remove ( aka removeUser )
  • isDirty ( to be called from Group(s) )
• User
  • isNameValid( userNameToAddIsValid )

Furthermore,

ko.computed() seems funny, I feel this really belongs to the C in MVC, the controller should create the model and pass that model to the ko.computeds.

userNameToAddIsValid and canAddUserName seem a mess, they can be merged.

So, in essence, this model is doing too much. Maybe your ugly models are doing too much as well. As mcknz mentioned, please post your awful code ;)