1
\$\begingroup\$

Looking at knockout for the first time, would like to know if I'm approaching it correctly.

Here's a live example: http://jsfiddle.net/fergal_doyle/xhAe8/1/

I have a "car" select, and an "other" input to allow values not present in the select box. When "other" is populated, it over-rides "car" by disabling it and a computed observable is set to the actual chosen car. Only when "other" is emptied, will "car" become enabled again.

HTML:

<label for="car">Car</label>
<select id="car" data-bind="options: cars, value: selectedCar, enable: checkCar"></select>

<label for="other">Other</label>
<input id="other" type="text" data-bind="value: other, valueUpdate:'afterkeydown'"/>

<p>Chosen value: <span data-bind="text: chosenCar"></span></p>

JS:

function viewModel(){
    var self = this;

    self.cars = ko.observableArray(["Chrysler", "Chevrolet", "Ford", "Lincoln"]);
    self.other = ko.observable("");
    self.selectedCar = ko.observable("");

    self.chosenCar = ko.computed(function() {
        return self.other() || self.selectedCar();
    }); 

    self.checkCar = ko.computed(function(){
        if(self.other())
            return false;
        else
            return true;
    });
}

ko.applyBindings(new viewModel());
\$\endgroup\$
  • 3
    \$\begingroup\$ if(self.other()) return false; else return true; is a bit verbose, I'd just write return !self.other(); \$\endgroup\$ – thejh Mar 31 '13 at 14:15
4
\$\begingroup\$

It looks fine to me. My only real criticism is your checkCar implementation. It ought to be written out as:

self.checkCar = ko.computed(function(){
    return !self.other();
});

On a minor note, you should case your view model name in PascalCase. By making constructors like that, they stand out better. Everything else should be in camelCase.


Other than that, you've made great use of the computed observables. A big mistake I see a lot on Stack Overflow IMHO is when I see people putting all that logic in the view.

I might change the name of the properties however to be more descriptive of what they represent. In particular, change checkCar to isSelectedCarOverridden.

Personally, I'd change it around a bit. I'd add an "Other" option in the cars list and make the Other car input enabled/visible based on that value.

Also, consider making cars not observable. If this is going to be a fixed list and you won't be dynamically adding to it, making it observable may be unnecessary. In fact, I'd pull that array out of the view model as it appears to be a constant value. That way if you have multiple view models instantiated in the future, they all can share the same instance.

var constants = {
    cars: ["Chrysler", "Chevrolet", "Ford", "Lincoln"]
};

function ViewModel() {
    var self = this;
    self.cars = constants.cars;
    // ...
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ What if I removed the checkCar function and did this: <select id="car" data-bind="options: cars, value: selectedCar, enable: !other()"></select> Is that considered putting logic in the view? \$\endgroup\$ – Fergal Apr 1 '13 at 13:26
  • 1
    \$\begingroup\$ I would say so. Any time your bindings doesn't look like binding: propertyName or binding: { 'prop': propertyName } and has some more complicated expression, I'd consider that putting logic into the view. All of that should be in a computed observable. It's better to keep the bindings in the view as simple as possible so it's easier to debug and you know where all the logic is (in the view model). \$\endgroup\$ – Jeff Mercado Apr 1 '13 at 13:33
  • \$\begingroup\$ Maybe I'm just reading it wrong but isn't it: camelCase and PascalCase? I.e. in camel casing the first word doesn't start with a capital but it does in Pascal casing (also called upper camel casing). \$\endgroup\$ – RobH Apr 2 '13 at 14:11
  • \$\begingroup\$ @RobH: Ah, you're right, I got it backwards. \$\endgroup\$ – Jeff Mercado Apr 2 '13 at 14:13
  • \$\begingroup\$ And while we're correcting things, in my comment above, "I'd consider putting that logic into the view model." \$\endgroup\$ – Jeff Mercado Apr 2 '13 at 14:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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