Tell me more ×
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: http://knockoutjs.com/examples/twitter.html and 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, a really bad ones!, specially with a more complicated business

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

share|improve this question
1  
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 – AbdelHadyMu Dec 24 '12 at 17:03
@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 – AbdelHadyMu 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 1 more comment

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

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

Browse other questions tagged or ask your own question.