Instantiating an object in another one

Question

Having an object, say Book which has a collection of other objects Page. So I instantiate pages from raw data passed to Book:

function Book(data){
    this.pages = [];
    var self = this;
    data.forEach(function(item){
        self.add(item);
    });
}

Book.prototype.add = function(data){
    this.pages.push(new Page(data));
}


function Page(data){
    // some validation code
    this.prop = data.prop;
}
Page.prototype...

from lectures on testability I heard that it is a bad practice to instantiate(use new) in another object. What is the right way to do the same?

If it is okay - is there any difference if I instantiate a new Page in add() method or pass to it as an object already(this.add(new Page(data)))?

*I heard that it is a bad practice to instantiate*. What lecture was that? That's a bizarre statement, perhaps you misunderstood it. If you need to create an object, you need to create it, wherever you are. — , Feb 10 '16 at 13:26
Do you want other people to be able to call `Book.prototype.add`? Would you expect them to pass in a constructed `Page` object, or to pass in the data in order to construct one. Depending on the answer to that, that's your answer - it really depends what interface you want to offer to consumers of your `Book`. — James Thorpe, Feb 10 '16 at 13:27
@torazaburo: I'll assume they meant that `book.add(new Page(…))` is better than `book.add(…)`. Which of course works better for a language that has static typing with interfaces. — Bergi, Feb 10 '16 at 13:31
Relying on new to construct objects is considered harmful because if you forget to, it doesn't bind `this` to a new object, but leaves it as possibly the global scope. This results in you throwing lots of garbage into the global scope, possibly messing up a lot of things. However, if you're running in strict mode, this is mostly mitigated as `this` is not automatically bound to the global scope — Joseph Young, Feb 10 '16 at 13:33
@JosephYoung: That sounds like "*relying on function calls is considered harmful because if you forget the arguments, it doesn't work*". [New is *not* harmful](http://stackoverflow.com/q/383402/1048572). — Bergi, Feb 10 '16 at 13:36
@bergi I agree, I was just simply stating the possible reason for the lecturer saying it's bad. Especially seeing as it's very easy to mitigate by checking if it was not called using `new`. That being said, the answer goes into the actual reason — Joseph Young, Feb 10 '16 at 13:39

score 5 · Accepted Answer · answered Feb 10 '16 at 13:31

The problem is when you want to write unit tests for you code.

Let's take an actual example. Some times later, your code is this one :

function Book(data){
    this.pages = [];
    var self = this;
    data.forEach(function(item){
        self.add(item);
    });
}

Book.prototype.add = function(data){
    var page = new Page(data);

    // Because the page need to know its book        
    page.book = this;

    this.pages.push(page);
}


function Page(data){
    // some validation code
    this.data = data;
}
Page.prototype...

Now you want to write an unit test for the add method, and you want to check that after you added a new page in your book, the page.book is the book. But with a code like that, you can't do it. Becase the page is created inside the method, you can't check anything.

Buf if we rewrite the code like that :

Book.prototype.add = function(page){
    page.book = this;
    this.pages.push(page);
}

We can now write a unit test :

describe('book.add', function() {

    it('should set the current book in the book property of the page', function() {
        var book = new Book(),
            page = new Page();

        book.add(page);

        expect(page.book).toBe(book);
    });

});

But, if you do not want to write any unit test like this one, there's no reason to not create the new page inside the method add.

really liked to see the back reference "// Because the page need to know its book "! — nik, Feb 10 '16 at 13:38

Instantiating an object in another one

1 Answers1