0

This is my first stab at trying to create a JavaScript object. I am attempting to extract properties from a very complex JSON response from a USGS REST service and save them as an object for later use.

What I am attempting to do is create an empty array as the first property of the object to later populate with instances of a custom object later, the last line of actual code. After digging around both the W3C, MDN and this site, I have yet to really come up with a solution.

Please feel free to not only offer solutions to the array issue, but also offer constructive criticism on the rest of the code. After all, I am trying to learn with this project.

// create site object
function Site(siteCode) {

    this.timeSeriesList = [];
    this.siteCode = siteCode
    this.downloadData = downloadData;

    // create timeSeries object
    function TimeSeries(siteCode, variableCode) {
        this.siteCode = siteCode;
        this.variableCode = variableCode;
        this.observations = [];
    }

    // create observation object
    function TimeSeriesObservation(stage, timeDate) {
        this.stage = stage;
        this.timeDate = timeDate;
    }

    // include the capability to download data automatically
    function downloadData() {

        // construct the url to get data
        // TODO: include the capability to change the date range, currently one week (P1W)
        var url = "http://waterservices.usgs.gov/nwis/iv/?format=json&sites=" + this.siteCode + "&period=P1W&parameterCd=00060,00065"

        // use jquery getJSON to download the data
        $.getJSON(url, function (data) {

            // timeSeries is a two item list, one for cfs and the other for feet
            // iterate these and create an object for each
            $(data.value.timeSeries).each(function () {

                // create a timeSeries object
                var thisTimeSeries = new TimeSeries(
                    this.siteCode,

                    // get the variable code, 65 for ft and 60 for cfs
                    this.variable.variableCode[0].value
                );

                // for every observation of the type at this site
                $(this.values[0].value).each(function () {

                    // add the observation to the list
                    thisTimeSeries.observations.push(new TimeSeriesObservation(

                        // observation stage or level
                        this.value,

                        // observation time
                        this.dateTime
                    ));
                });

                // add the timeSeries instance to the object list
                this.timeSeriesList.push(thisTimeSeries);
            });
        });
    }
}

If you would like to view the JSON I am using for testing, you can find it here: http://waterservices.usgs.gov/nwis/iv/?format=json&sites=03479000&period=P1W&parameterCd=00060,00065

Thank you in advance for your time and mentoring!

knu2xs
  • 910
  • 2
  • 11
  • 23
  • possible duplicate of [How to access the correct \`this\` / context inside a callback?](http://stackoverflow.com/questions/20279484/how-to-access-the-correct-this-context-inside-a-callback) – Felix Kling Dec 11 '13 at 18:48
  • @FelixKling yes, probably a duplicate, except that the OP doesn't know that that's his problem... – Alnitak Dec 11 '13 at 18:53

1 Answers1

2

Your primary problem is that this inside the AJAX callback isn't your object, it's variably either the jqXHR object created by jQuery or the current element that you're iterating over with .each.

The simplest solution is to create another reference to this in the higher level lexical scope that may be accessed from inside the inner functions:

var self = this;
$.getJSON(..., function() {
     // use self inside to refer to your object
});
Alnitak
  • 334,560
  • 70
  • 407
  • 495
  • Thank you so much for the explanation including the code snippet! It works and also clarifies an inconsistency not making much sense while cobbling this together. – knu2xs Dec 11 '13 at 19:14
  • Wrong way of doing this as you are creating a closure of the reference of your object within the callback function. Binding the function scope will be correct if you use jquery look into $.proxy – elmuchacho Dec 12 '13 at 09:53
  • 1
    @elmuchacho and how do you think `$.proxy` and `Function.bind` work? They both create a new closure on the supplied parameter... – Alnitak Dec 12 '13 at 10:03
  • Managed closure by passing the context as an arguments to the function yes, this are completely different. Why do you think Function.bind as been added to the Javascript language? var self is just bad practice but if you want to recommend it, go for it. When you write object oriented javascript you have to maintain a consistency in the use of keyword this and it should always refer to the instance of the class you are in and not the scope of the function executed. Code like the one posted above are un-maintainable and scale really badly. – elmuchacho Dec 12 '13 at 10:25
  • @elmuchacho I strongly disagree. `this` is a "magic" variable within JS and many libraries (and users) expect it to work a particular way, for example by default it's the clicked element inside an event handler, it's the current element in `$.each`, etc. IMHO in most cases it causes far less confusion to take advantage of the lexical scope than to arbitrarily "force" `this` to be something that it isn't. In the OP's case he has _two_ functions overwriting `this` which would need to by bound. A single reference to `self` is _far_ simpler, and at least as efficient, if not more so. – Alnitak Dec 12 '13 at 10:31
  • 2 things: 1) jsperf your statement it will prove you wrong 2) Read https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/this – elmuchacho Dec 12 '13 at 12:34
  • @elmuchacho nothing in that link backs up your argument – Alnitak Dec 12 '13 at 12:48
  • Lol Really I wonder where do they recommend you to declare a variable to hold the reference of the context of the object you've instantiated part of a constructor. Obviously you can read that doesn't mean you understand! – elmuchacho Dec 12 '13 at 14:09
  • 2
    They don't recommend it, but nor do they say _not to_, it's just not addressed. And they specifically call out some of the same cases I did for when `this` has an expected behaviour which your model would break. I understand perfectly that you think I'm wrong. But your argument doesn't hold water and goes against a practise that is almost universally used. Your assertion that `this` must _always_ refer to your outer object when the language wasn't designed that way is IMHO ridiculous. – Alnitak Dec 12 '13 at 14:21
  • @elmuchacho if you feel this answer is poor and you can provide a better solution then you probably should do that. – rlemon Dec 12 '13 at 14:25
  • @elmuchacho *"Obviously you can read that doesn't mean you understand!"*. The fact you disagree with Alnitak doesn't allow you to be insulting. By the way, most experienced JS coders would probably agree with Alnitak that binding isn't always the way and using closures to refer to external context is fine. – Denys Séguret Dec 12 '13 at 14:30
  • A small note : I would generally use a more semantic name than `self`. This is especially useful if you go deeper and need to refer to more than one context. – Denys Séguret Dec 12 '13 at 14:33
  • @dystroy I have been trying to get the young kids excited about my code, So I use `var dat = this;` then I can reference `dat` var inside the function. – rlemon Dec 12 '13 at 15:27