2
\$\begingroup\$

This is my first time using knockout. I'm reading/writing a JSON file with a fair bit of nested data. I had no problem creating the ViewModel to just write to the file, but now that I'm reading the data into my model also, I've had to write a few if/else statements to handle the different cases. Not sure if this is the right way to do it.

I've got a ViewModel with a function to add a new record, and to save the data. The 3 functions above are used for mapping the data.

function Survey(name, panes, tester) 
{
  this.name = ko.observable(name);
  this.panes = ko.observableArray([new Pane(panes)]);
  this.tester = ko.observable(tester);
}

function Pane(panes) 
{
  //var panesCount = panes.length;
  //console.log(panes[0].type);
  if (panes) {
    this.type = ko.observable(panes[0].type);
    this.head = ko.observable(panes[0].head);
    this.content = ko.observable(panes[0].content);
    this.options = ko.observableArray([new PaneOptions(panes[0].options)]);
  }
  else {
    this.type = ko.observable();
    this.head = ko.observable();
    this.content = ko.observable();
    this.options = ko.observableArray([new PaneOptions()]);
  }
}

function PaneOptions(options) 
{
  //console.log(options);
  if (options) {
    this.data = ko.observable(options[0].data);
    this.target = ko.observable(options[0].target);
  }
  else {
    this.data = ko.observable();
    this.target = ko.observable();
  }
}


function SurveyViewModel()
{
  var self = this;
  self.surveys = ko.observableArray([]);


  $.getJSON("create/fetch", function(data) { 
    //console.log(data.surveys[0].name);
    if (data) { // check if json file is empty
      for (var i=0; i < data.surveys.length; i++) {
        console.log(i);
        self.surveys.push(new Survey(data.surveys[i].name, data.surveys[i].panes, data.surveys[i].tester))
      }
    }
  });    

  self.addSurvey = function () 
  {
    self.surveys.push(new Survey());
  };

  self.saveUserData = function() 
  { 
    var data_to_send = ko.toJSON(self); 
    $.post("create/save", data_to_send, function(data) { 
      console.log("Your data has been posted to the server!"); 
    }); 
  }

}

var vm = new SurveyViewModel();
ko.applyBindings(vm);

Here you can see the JSON representation:

{
    "surveys": [
        {
            "name": "My First Survey1",
            "panes": [
                {
                    "type": "question1",
                    "head": "Heading Text1",
                    "content": "multichoice1",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default"
        },
        {
            "name": "My Second Survey2",
            "panes": [
                {
                    "type": "response2",
                    "head": "Heading Text2",
                    "content": "multichoice2",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default2"
        },
        {
            "name": "My Third Survey3",
            "panes": [
                {
                    "type": "thanks3",
                    "head": "Heading Text3",
                    "content": "multichoice3",
                    "options": [
                        {
                            "data": "Time",
                            "target": 2
                        }
                    ]
                }
            ],
            "tester": "default3"
        }
    ]
}

Any input is greatly appreciated!

\$\endgroup\$
2
\$\begingroup\$

Overall, it looks really good and serves as a pretty great model of what Knockout models should look like, but, internally there are some strange things here like arrays that you are using [0] only on (this could be the result of poor data format via your backend web services) and some ambiguity between Panes, Pane, and PaneOptions.

(1) Use the guard or ternary operator to create defaults

This will help you avoid some of these logic blocks in all of your constructors.

(2) Fix ambiguity with singular and plural constructors (Planes, Plane)

The relationship between Panes and Pane are a little strange! In the Survey constructor, you are create a variable called panes (this.panes) and then instantiating and passing a new Pane to which you are passing an array of panes? A little bit recursive. In the Pane constructor I would not expect to take in panes in the arguments. I think your intention is for the Survey to create an array of panes, like this:

function Survey(name, panes, tester) 
{
  var _this = this;

  // 1
  this.name = ko.observable(name || '');
  // 2
  this.panes = ko.observableArray();
  this.tester = ko.observable(tester);

  if(panes){
    panes.forEach(function(pane){
      _this.panes.push(new Pane(pane));
    });
  }
}

(3) Fix improper use of arrays with fixed [0] element

Why is Panes plural in the Pane constructor, if you are only using array item 0? I think the Pane constructor should just take in an parameter like options which would be used to return an individual pane:

function Pane(config) 
{
  this.type = ko.observable(config.type);
  this.head = ko.observable(config.head);
  this.content = ko.observable(config.content);
  this.options = ko.observableArray([new PaneOptions(config.options)]);

  /*
   * You could return "this" but in JS that is the default behavior.
   */
}

Additionally, this will help you avoid JS errors when you might attempt to access a null array. For example, if panes in panes[0].options were null, you would have an error at pane[0] before you even got to the options property.

I am not sure why you are operating on options[0] here - at the point when you are instantiating a Pane, you should be passing in a config (see above) which has an options property, which will most likely be an object like { data: "", target: "" } since that is what you are expecting in the PaneOptions constructor:

function PaneOptions(options) 
{
  // Setup defaults with ternary operator
  this.data = ko.observable(options ? options.data : '');
  this.target = ko.observable(options ? options.target : '');
}

Applicable original code with numbered callouts corresponding to items above

function Survey(name, panes, tester) 
{
  // 1
  this.name = ko.observable(name || '');
  // 2
  this.panes = ko.observableArray([new Pane(panes)]);
  this.tester = ko.observable(tester);
}

// 2
function Pane(panes) 
{
  if (panes) {
    // 3
    this.type = ko.observable(panes[0].type);
    this.head = ko.observable(panes[0].head);
    this.content = ko.observable(panes[0].content);
    this.options = ko.observableArray([new PaneOptions(panes[0].options)]);
  }
  else {
    this.type = ko.observable();
    this.head = ko.observable();
    this.content = ko.observable();
    this.options = ko.observableArray([new PaneOptions()]);
  }
}

function PaneOptions(options) 
{
  if (options) {
    // 3
    this.data = ko.observable(options[0].data);
    this.target = ko.observable(options[0].target);
  }
  else {
    this.data = ko.observable();
    this.target = ko.observable();
  }
}
\$\endgroup\$

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.