3

In his "Good Parts," Crockford suggests that 'new' should never be used. To follow that rule, how would you refactor the following code?

function Range(from, to) { 
    this.from = from; 
    this.to = to; 
} 

Range.prototype = { 
    includes: function(x) {
        return this.from <= x && x <= this.to;
    }, 

    foreach: function(f) { 
        for(var x = Math.ceil(this.from); x <= this.to; x++) f(x);
    },

    toString: function() { 
        return "(" + this.from + "..." + this.to + ")"; 
    } 
};

// Here are example uses of a range object 
var r = new Range(1,3); // Create a range object 
r.includes(2); // => true: 2 is in the range 
r.foreach(console.log); // Prints 1 2 3

I spotted his additional advice, but it wasn't clear how to apply it in this (presumably very common) case. Would he propose to create a factory function that contains a giant object literal? If yes, isn't that inefficient? ISTM that such a factory function, upon each invocation, creates duplicate functions. In other words, there is no one prototype holding shared custom methods.

It seems something is left unsaid in his advice, I'm hoping someone can clear it up.

Brent Arias
  • 29,277
  • 40
  • 133
  • 234
  • 6
    `new` should never be used? Can't agree with that one at all. –  Mar 05 '12 at 03:44
  • Why don't you read this thread: http://stackoverflow.com/questions/383402/is-javascript-s-new-keyword-considered-harmful – Murray McDonald Mar 05 '12 at 03:47
  • 1
    Keep in mind that Crockford's "suggestions" are just that, and please take them with a grain of salt. – Jesse Good Mar 05 '12 at 03:49
  • Indeed, can't agree with that -- just use whatever works best for you. – casablanca Mar 05 '12 at 03:49
  • The additional advice you linked to says _"So the rule is simple: The only time we should use the `new` operator is to invoke a pseudoclassical Constructor function. When calling a Constructor function, the use of `new` is mandatory."_ - so Crockford isn't really saying to _never_ use it, he's saying to use it only when appropriate. As far as I'm concerned the code in your question is perfectly fine use of `new` and doesn't need to be refactored. – nnnnnn Mar 05 '12 at 04:26

3 Answers3

2

Here I am showing how you can achieve this without using new

Range = function(from, to) {

    function includes(x) {
        return this.from <= x && x <= this.to;
    }

    function foreach(f) {
        for (var x = Math.ceil(this.from); x <= this.to; x++) f(x);
    }

    function toString(){
        return "(" + this.from + "..." + this.to + ")";
    }

    return {
        from: from,
        to: to,
        includes: includes,
        foreach:  foreach,
        toString: toString
    };
};

var r = Range(1, 3);
console.log(r.includes(2)); // => true: 2 is in the range
r.foreach(console.log); // Prints 1 2 3

This is just an example, but I would follow what @nnnnnn is saying - "use it only when appropriate. As far as I'm concerned the code in your question is perfectly fine use of new and doesn't need to be refactored."

EDIT:

The code given below will avoid creating duplicate instances of functions

Range = function(from, to) {
    return {
        from: from,
        to: to,
        includes: Range.includes,
        foreach:  Range.foreach,
        toString: Range.toString
    };
};

Range.includes = function(x) {
    return this.from <= x && x <= this.to;
}

Range.foreach = function (f) {
    for (var x = Math.ceil(this.from); x <= this.to; x++) f(x);
}

Range.toString = function() {
    return "(" + this.from + "..." + this.to + ")";
}
Diode
  • 24,570
  • 8
  • 40
  • 51
  • But doesn't this factory function create duplicate definitions of the methods each time it is called? Wouldn't it be less efficient (even if negligably) from the 'new' approach? – Brent Arias Mar 05 '12 at 18:35
  • I think what I understood from your question was wrong when you mentioned "duplicate". Now I understand correctly. Yes, you are correct. My first example will dynamically create those functions on each call of `Range`. To avoid this we have to use an object. But these functions will have access to everything in the scope where they are declared. – Diode Mar 06 '12 at 06:00
1

I think he would suggest doing something like this:

function Range(from, to) {
  if (!(this instanceof Range)) {
    return new Range(from, to);  // Yes, the new operator is used here, but...
  }

  this.from = from;
  this.to = to;
}

// ... now, the rest of the world can create ranges WITHOUT the new operator:
var my_range = Range(0, 1);
allyourcode
  • 21,871
  • 18
  • 78
  • 106
  • maybe it's just me but using `instanceof` it's far worse that using `new` (even if I don't see anything wrong with using new) – nicosantangelo Mar 05 '12 at 03:52
  • I understand the appeal of this pattern, and I might consider using it if I was providing a JS library for other people and I was concerned that they might ignore any documentation I provided and incorrectly call my constructor functions without `new`, but I don't think I'd use it in my own code: if I want a function that acts as a constructor I expect client code to call it with `new`. If I want a function that acts as a factory then I expect it to be named accordingly, e.g., `getRange()` rather than `Range()`. – nnnnnn Mar 05 '12 at 04:33
0

I love this technique, I refer to it as guaranteed instances:

var Range = function fn(from, to) {
    if (!(this instanceof fn)) {
        return new fn(from, to);
    };
    this.from = from; 
    this.to = to; 
} 

Range.prototype = { 
    includes: function(x) {
        return this.from <= x && x <= this.to;
    }, 

    foreach: function(f) { 
        for(var x = Math.ceil(this.from); x <= this.to; x++) f(x);
    },

    toString: function() { 
        return "(" + this.from + "..." + this.to + ")"; 
    } 
};

// Here are example uses of a range object 
var r = new Range(1,3); // Create a range object 
r.includes(2); // => true: 2 is in the range 
r.foreach(console.log); // Prints 1 2 3
Eli
  • 17,397
  • 4
  • 36
  • 49