Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
2  
This site is for reviewing code. While your statement may be true, a working illustration with actual code would help and follows the FAQ. If you have some code to add to demonstrate your point, then you should do so, otherwise this will get closed as being off topic. –  mseancole Dec 24 '12 at 16:54
    
@mseancole sorry, I was talking about the normal way knockout.js is handling his code, and how we can use his ViewModel & Model in a cleaner way, anyway I included a code demonstrating how knockout.js can make code worse –  Abdel Hady Dec 24 '12 at 17:03
1  
@AbdelHadyMu Post an excerpt of your own code. It's easier to discuss a specific, real example than to talk about knockout in the abstract. –  mcknz Dec 24 '12 at 17:52
    
@mcknz I do understand your point, but I really mean that Knockout.js do have a readability problem from defining this.foo = function(){} (and all in one viewModel where you can't reach the viewModel parameters easily) to defining ViewModel with its Models in the same file, for example I have tried to separate the inline functions, but then knockout didn't work, and so on –  Abdel Hady Dec 24 '12 at 20:28
1  
this is not ugly code , this is proper MVVM implementation , can you please tell us which part of code is ugly or you want to refactor –  paritosh Dec 25 '12 at 15:55
show 2 more comments

2 Answers 2

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.

share|improve this answer
    
this is really a good idea, I have heard this from other JavaScript programmers as well. –  Simon André Forsberg Jan 31 at 19:48
    
You'll get an up-vote from me as soon as I get my votes back! –  Simon André Forsberg Jan 31 at 19:49
    
If it is clear that we want to refer to the current object instance, then self instead of this may be a good solution, as that's the default convention in many languages (Smalltalk, Python, Perl, …). –  amon Jan 31 at 20:04
1  
My point for this was to de-genericize "this". "self" is usable also but I think the use of both of them can be quickly confusing in any situation where the code is written such that contexts are nested. I don't like to have to guess. Here's an example: jsfiddle.net/sZ4W8 –  codenoire Feb 1 at 0:17
add comment

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 ;)

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.