A bug involving JavaScript's object structure

Question

I'm writing a whole client-side system in JavaScript. I have about 4 .js files, but there is no need to post code from them, nor the html.

Anyway, before explaining the problem, I'll try to explain what is the code about, and the code itself.

Basically, it's a finite state machine in a design level. For now, I only have 3 states: the initial, a transition and the final state (the state machine, for now, is a queue). The code is this:

var state = 0

var IntInsert = //my object that represents a namespace, or a set of functions
{
    insertCtrl : function() //main function: it calls the others
    {
        if (lookup()) return
        if (!state) state = 1
        var key = document.getElementById("newegg").value
        switch(state)
        {
            case 1:
                this.preInsert(key)
                break
            case 2:
                this.firstInsert(key)
                break
        }
    },

    preInsert : function(key)
    {
        $("#terminal").css("color", "green")
        $("#terminal").text("Inserting element '" + String(key) + "'")
        $("#t2cell" + String(cuckoohash.h2(key))).css("background-color", "White")
        $("button").prop("disabled", true)
        $("input").prop("disabled", true)
        $("#nextstep").prop("disabled", false)
        state++
    },

    firstInsert : function(key)
    {
        key = [document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML, document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML = key][0] // key <-> t1[h1(key)]
        if (!key)
        {
            $("#t1cell" + String(cuckoohash.h1(document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML))).css("background-color", "LightGreen")
            this.finishedInsert()
        }
    },

    finishedInsert : function()
    {
        $("#terminal").css("color", "green")
        $("#terminal").text("Element '" + String(key) + "' inserted")
    }
}

In the line this.firstInsert(key), a bug happens. Firebug tells me this: TypeError: this.firstInsert is not a function. This is weird, given that it's defined as a function and Firebug itself represents it as a function in it's high-level panel.

If I take the function out of the object and use it as a global function, there is no error at all. That's why I believe the whole semantics of the code can be ignored in order to answer my question, which is: why is this happening inside the object? What is it that I'm missing?

After reading the acknowledged answer, I realized it's important to state that the whole point of the code being in an object is to create a namespace for the whole state machine code I'm creating, so I definitely could put the state variable inside it. The idea is to avoid having large code base with global variables and functions.

It's probably because of how you're calling the `.insertCtrl()` method. In Javascript, `this` is set according to how a function is called (not how it is declared). If you show us the calling code, we can likely explain why it's going wrong. See [this answer](http://stackoverflow.com/questions/28016664/when-you-pass-this-as-an-argument/28016676#28016676) for further info on how `this` gets set according to how something is called. — jfriend00, Apr 22 '16 at 17:10
Also, since `IntInsert` is a singleton (there's only ever one of them), you don't have to use `this` to reference its instance data or methods. You could just use `IntInsert.firstInsert(key)` instead of `this.firstInsert(key)`. — jfriend00, Apr 22 '16 at 17:12
If what you implemented is the answer you accepted, that's far from the simplest way to solve your problem. It adds way more complexity than is necessary for a simple singleton object and since I generally believe the best code is the simplest solution that meets your needs, I would not recommend that for a singleton. — jfriend00, Apr 23 '16 at 00:18
I posted my comments into an answer because I think they are simpler and more appropriate for a singleton object than the answer you accepted. — jfriend00, Apr 23 '16 at 00:22

G. Stoynev · Answer 1 · 2016-04-22T17:18:29.650

1

The reason you are seeing the error is most likely because at runtime 'this' is changing to an object that doesn't have your function. Try restructuring your code and "closing" on this like this:

var IntInsert = function()
{
    var self = this;
    ...
    var insertCtrl = function() //main function: it calls the others
    {
        ... 
        case 2:
            self.firstInsert(key)
            break
        ...
    }
    ...

    var firstInsert = function(key)
    {
        ...
    }

    return {
        insertCtrl: insertCtrl,
        firstInsert: firstInsert
    }
}

... or something similar that would fit your needs.

edited Apr 22 '16 at 17:18

answered Apr 22 '16 at 17:09

G. Stoynev

7,389
6
38
49

1

@JudismarJunior - I don't know why this is the accepted answer because this only works if `IntInsert()` is used as a constructor which is not how the OP is using it at all. Further, there's no reason to make something that only needs to be a singleton into something more complicated. Just no reason. The real answer here is either for the user to just use it as a singleton and just use the `IntInsert` prefix instead of `this` or fix the calling code to not tromp on the value of `this`. There's no reason for this level of complexity for such a simple problem. – jfriend00 Apr 23 '16 at 00:27
@jfriend00 Thank you, again. In fact, I most likely accepted this due to the explanation on the problem (one phrase), not the solution itself. I didn't use it, exactly. I wanted to learn what the problem is, and this answer did. I saw you wrote an answer yourself, I'm definitely reading it, because I'm suppose to accept an answer that actually solves the problem, not only satisfies my doubts. – Judismar Arpini Junior Apr 23 '16 at 13:32

score 1 · Answer 2 · answered Apr 22 '16 at 17:11

1

I think you are having issue with what this is when you are calling your function. You have not shown how you are calling the function but most probably when you log this before that line, it won't be your IntInsert object but may be global Window object

answered Apr 22 '16 at 17:11

Bhabishya Kumar

731
3
7

I appreciate your attention and explanation over this! – Judismar Arpini Junior Apr 23 '16 at 00:16

score 1 · Accepted Answer · edited May 23 '17 at 12:23

Your issue is likely caused by how you are calling your methods. In Javascript, the value of this is determined by how a method is called, not by how it is declared. You don't show the calling code (where the problem likely is), but if you are calling one of your methods via some sort of callback, then you can easily lose the proper method call that will preserve the value of this for you. See this other answer for more info on how the value of this is determined based on how a function is called.

There are many different possible solutions.

Since your object is a singleton (there's only ever just one of them and you've given that one instance a name), I'd suggest that the simplest way to solve your problem is to just refer to the named singleton IntInsert rather than refer to this as that will solve any issues with the value of this in your methods.

You can do that like this:

var state = 0

var IntInsert = //my object that represents a namespace, or a set of functions
{
    insertCtrl : function() //main function: it calls the others
    {
        if (lookup()) return
        if (!state) state = 1
        var key = document.getElementById("newegg").value
        switch(state)
        {
            case 1:
                IntInsert.preInsert(key)
                break
            case 2:
                IntInsert.firstInsert(key)
                break
        }
    },

    preInsert : function(key)
    {
        $("#terminal").css("color", "green")
        $("#terminal").text("Inserting element '" + String(key) + "'")
        $("#t2cell" + String(cuckoohash.h2(key))).css("background-color", "White")
        $("button").prop("disabled", true)
        $("input").prop("disabled", true)
        $("#nextstep").prop("disabled", false)
        state++
    },

    firstInsert : function(key)
    {
        key = [document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML, document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML = key][0] // key <-> t1[h1(key)]
        if (!key)
        {
            $("#t1cell" + String(cuckoohash.h1(document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML))).css("background-color", "LightGreen")
            IntInsert.finishedInsert()
        }
    },

    finishedInsert : function()
    {
        $("#terminal").css("color", "green")
        $("#terminal").text("Element '" + String(key) + "' inserted")
    }
}

Some Other Comments About the Code:

Leaving out semi-colons at the end of each statement may expose you to some accidental bugs. It seems a less than desirable practice to me. The first time you see such a bug, you probably be confused for quite awhile why your code isn't working as intended too.

You can chain in jQuery so instead of this:

    $("#terminal").css("color", "green");
    $("#terminal").text("Element '" + String(key) + "' inserted");

You can use this:

    $("#terminal").css("color", "green").text("Element '" + String(key) + "' inserted");

In Javascript, you rarely need to explicitly cast to a string with String(key), so instead of this:
```
    $("#terminal").text("Element '" + String(key) + "' inserted");
```
You can use this:
```
    $("#terminal").text("Element '" + key + "' inserted");
```
Adding anything to a string will auto-convert that to a string for your automatically.
You went to the trouble to make a singleton namespace object IntInsert, yet you declare your variable state that is not in that namespace object. It seems you should do one of the other, not some odd combination of the two. You either want relevant things in your namespace object or not and such a generically named variable as state is ripe for a naming conflict. I would think it should be in your object.
It is odd to use a mixture of jQuery selectors like $("#terminal") and document.getElementById("newegg"). Code is usually cleaner if you stick with one model or the other. If you're using jQuery for other reasons, then code like:
```
var key = document.getElementById("newegg").value;
```
can be this in jQuery:
```
var key = $("#newegg").val();
```
In .firstInsert(), you declare an argument named key, but then in the first line of code in that method, you assign to key so the code is just misleading. You declare an argument, but are never using that argument if it is passed in. You should either use the argument or change the key variable to be just a local variable declaration, not a function argument.
In .finishedInsert(), you refer to a variable named key, but there is no such argument or variable by that name.
In .firstInsert(), this line of code is just bizarre:
```
var key = [document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML, document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML = key][0]; 
```
You appear to be declaring an array with two elements in it, then assigning just the first element to key. The second element in the array is an assignment to a .innerHTML property. This is simply a weird and misleading way to write this code. It should be broken up into multiple lines of code.
The first line of code in .firstInsert() appears to be referring to some sort of global definition of a variable named key which does not appear anywhere in your code. That is likely a bad idea. The key you use here should either be in your namespace object or should be passed as an argument to .firstInsert(xxx).

Here's a version of your code with as many of those items above fixed/modified and FIXME comments where things are still unexplained:

//my object that represents a namespace, or a set of functions
var IntInsert = {
    //main function: it calls the others
    state: 0,
    insertCtrl: function () {
        if (lookup()) return;
        if (!IntInsert.state) IntInsert.state = 1;
        var key = $("#newegg").val();
        switch (IntInsert.state) {
        case 1:
            IntInsert.preInsert(key);
            break;
        case 2:
            IntInsert.firstInsert(key);
            break;
        }
    },

    preInsert: function (key) {
        $("#terminal").css("color", "green").text("Inserting element '" + key + "'");
        $("#t2cell" + cuckoohash.h2(key)).css("background-color", "White");
        $("button").prop("disabled", true);
        $("input").prop("disabled", true);
        $("#nextstep").prop("disabled", false);
        IntInsert.state++;
    },

    firstInsert: function () {
        // key <-> t1[h1(key)]
        // FIXME here: There is no variable named key anywhere
        document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML = key;
        if (!key) {
            $("#t1cell" + cuckoohash.h1(document.getElementById("t1").rows[cuckoohash.h1(key)].cells[0].innerHTML)).css("background-color", "LightGreen");
            IntInsert.finishedInsert();
        }
    },

    finishedInsert: function () {
        // FIXME here: There is no variable named key anywhere
        $("#terminal").css("color", "green").text("Element '" + key + "' inserted");
    }
};

Thanks a bunch, @jfriend00. Your answer does a full debugging and code-optimization as well. The state being outside of the object is something I realized when writing the question, but I was so concerned about the bug I actually ignored it. The semicolon tip is actually comforting, I'm used to it in C and usually write JavaScript with it "accidentally", then I erase it. All the other tips are totally useful to me as a JavaScript programmer with JQuery. — Judismar Arpini Junior, Apr 23 '16 at 13:42
Oh, also... you solve the problem by the same singleton tip you gave me in comment, so anyone who reads this will be satisfied at that point (although I encouraje to read everything, and the link as well). — Judismar Arpini Junior, Apr 23 '16 at 13:44
Finally: to give more insight on my problem, I called the InsertCtrl method as a callback function. It wass called everytime I pressed a button (using JQuery structures). — Judismar Arpini Junior, Apr 23 '16 at 14:37

A bug involving JavaScript's object structure

3 Answers3