Cannot read property of undefined of function inside loop

Question

I have the "error: Cannot read property 'shorten' of undefined" error when running my test. I want my loop to run the shorten function to check if the string is longer then 20 characters, and if so limit it to that.

  function ListView(list) {
    this.list = list;
    this.converted;
  }

  ListView.prototype.convert = function() {
    var output = [];
    this.list.notelist.forEach(function(element) {
      this.shorten(element);
      output += "<li><div>" + element.text + "</div></li>";
    });
    this.converted = "<ul>" + output + "</ul>";
  };

  ListView.prototype.shorten = function(string) {
    if (string.length > 20) {
      return string.substring(0, 20);
    }
    return string;
  };

list is from another constructor but i mocked it with;

var mockList = { notelist: [{ text: "hello" }, { text: "goodbye" }] };

FYI, your `var output` should be initialized to an empty string, not an empty array. It works because of an empty array will be coerced to an empty string when the concatenation happens, but it's not really right. The `+=` doesn't add anything to the array. It just destroys it. — , Dec 30 '17 at 19:01
...and this would be nicer with `.reduce()`, like this: `this.converted = this.list.notelist.reduce((s, el) => s + "

Ken Kin · Answer 1 · 2017-12-30T20:19:01.013

There are serveral problems with your code:

enter image description here

You encountered the what is this problem which is a very common problem with beginners, have a look at this link. In the anonymous function body of function (element) { .. it doesn't obtain the context of your custom type, thus this is a reference to your browser window.
Your shorten method is called with a different usage in its semantics. You did not take what it returns, but element is not modified at all with the method.

So, let's try to correct the code for what you attempt to do:

<script>
    function ListView(list) {
        this.list = list;
        this.converted;
    }

    ListView.prototype.convert = function () {
        var output = [];

        var that = this;
        this.list.notelist.forEach(function (element) {
            that.shorten(element);
            output += "<li><div>" + element.text + "</div></li>";
        });
        this.converted = "<ul>" + output + "</ul>";
    };

    ListView.prototype.shorten = function (el) {
        var string = el.text;

        if (string.length > 20) {
            el.text = string.substring(0, 20);
        }
    };

    var mockList = { notelist: [{ text: "hello" }, { text: "goodbye0123456789012345" }] };
    var listview1 = new ListView(mockList);
    listview1.convert();
    alert(listview1.converted);

</script>

goodbye0123456789012345 is modified intentionally for the test, in the result it will be shorten as goodbye0123456789012.

score 1 · Answer 2 · answered Dec 30 '17 at 18:37

1

You lost the this binding in the forEach Try:

  ListView.prototype.convert = function() {
    var output = [];
    this.list.notelist.forEach(function(element) {
      this.shorten(element);
      output += "<li><div>" + element.text + "</div></li>";
    }.bind(this));
    this.converted = "<ul>" + output + "</ul>";
  };

or

this.list.notelist.forEach((element) => {
  this.shorten(element);
  output += "<li><div>" + element.text + "</div></li>";
});

Similar to: Passing scope to forEach

answered Dec 30 '17 at 18:37

The Dembinski

1,469
1
15
24

1

You don't need `.bind()` for the first example. You can just pass `this` as the second argument to `.forEach()`, like `this.list.notelist.forEach(func..., this)` – Dec 30 '17 at 18:41
You can also do that. Your medal is in the mail. – The Dembinski Dec 31 '17 at 04:03
Well, it just makes sense, right? Why would anyone construct a new function object if they know that they can set the `this` via the `.forEach()` parameter? – Dec 31 '17 at 14:22
Oh no, I totally agree w. you. Hence the up vote. However, one thing I do prefer - in Javascript particularly - is sticking to patterns people find more idiomatic? Your suggestion - while solid - isn't something I've seen in the wild and would piss off my coworkers. It works. Please use it. I posted what I feel would benefit to OP most for the theater of discussion at hand. Additionally - the link I provided shows your suggestion as an alternative. – The Dembinski Dec 31 '17 at 16:57

score 0 · Answer 3 · answered Dec 30 '17 at 18:37

0

Try by binding this

this.list.notelist.forEach(function(element) {
      this.shorten(element);
      output += "<li><div>" + element.text + "</div></li>";
    }.bind(this));

answered Dec 30 '17 at 18:37

brk

48,835
10
56
78

score -2 · Answer 4 · answered Dec 30 '17 at 18:38

You can use an ES6 arrow function in this case to ensure that this is maintained within your function.

this.list.notelist.forEach((element) => {
  this.shorten(element);
  output += "<li><div>" + element.text + "</div></li>";
});

Remember to check the availability of arrow functions on browsers. If you want a more widely supported solution use the bind function like others have mentioned.

XCS · Answer 5 · 2017-12-30T18:46:01.693

-2

forEach accepts a second argument which is the thisArg, by default it is undefined.

Try this:

   this.list.notelist.forEach(function(element) {
      this.shorten(element);
      output += "<li><div>" + element.text + "</div></li>";
    }, this); // <-- note the `this` argument passed here

edited Dec 30 '17 at 18:46

answered Dec 30 '17 at 18:40

XCS

27,244
26
101
151

2

Why `Don't listen to other answers suggesting extra binding or arrow functions`? There is no reason why the other answers aren't correct. Why is your answer any better than the others? Just different ways to achieve the same thing. – Charlie Fish Dec 30 '17 at 18:42
1

I agree about the `.bind()` functions, but dismissing arrow functions seems silly. This is a primary reason for including them in the language. – Dec 30 '17 at 18:43
@CharlieFish How are the other answers better in any way? This is better as you write less code, it is more efficient and uses the language syntax as it was meant to be used. If you solve each JavaScript problem by creating extra scopes you will eventually have a lot of memory leaks and garbage collections in your program. It's a bad thing to teach other language anti-patterns. – XCS Dec 30 '17 at 18:44
@Cristy I'm not saying they are better. All I'm saying is don't dismiss the other answers by saying `Don't listen to other answers suggesting extra binding or arrow functions`. The other answers are legit as well. They will work just as well. – Charlie Fish Dec 30 '17 at 18:45
1

@rockstar I am just dismissing arrow functions and binding for this specific case. I will remove that line from the answer anyway, as you are right, if you don't mind a bit more overhead than arrow functions are a perfectly reasonable solution. – XCS Dec 30 '17 at 18:45
1

@Cristy: Arrow functions don't create more overhead. If anything, they're a little lighter. Since they're a newer syntax, they may be less optimized at the moment (or maybe not), but I don't know where extra overhead would come from. – Dec 30 '17 at 18:47
@rockstar Arrow functions are just syntactic sugar, behind the scenes the JavaScript engine just compiles it to the same thing as a normal function with a bound context. They are heavier than regular functions, but as heavy as regular functions with a bound context. – XCS Dec 30 '17 at 18:49
In my opinion the biggest downside to arrow functions is lack of support on older browsers. – Charlie Fish Dec 30 '17 at 18:49
1

@Cristy: I think you may be making assumptions. Mozilla did this early on because it was a simple way to get a preliminary implementation going, but the spec defines an arrow function as, in part, a function that does not define its own `this` binding. So it uses the `this` of its enclosing lexical environment. While it's possible that there could be a little overhead for the closure, that's another thing that would seem possible to optimize. – Dec 30 '17 at 18:52
Thanks everyone for the help! Very interesting stuff, multiple ways of solving the problem. – pyan Dec 30 '17 at 19:31

Cannot read property of undefined of function inside loop

5 Answers5