addyosmani / essential-js-design-patterns

Repo for my 'Learning JavaScript Design Patterns' book
http://addyosmani.com/resources/essentialjsdesignpatterns/book/
4.83k stars 792 forks source link

Question about the vehicle factory example #224

Open alvarocastro opened 5 years ago

alvarocastro commented 5 years ago

Won't this line of the example change the default vehicle class for the next createVehicle call?

Here is the code

Brought the code here. I put a comment to point the line I'm taling about, it's near the end. ```js // A constructor for defining new cars function Car( options ) { // some defaults this.doors = options.doors || 4; this.state = options.state || "brand new"; this.color = options.color || "silver"; } // A constructor for defining new trucks function Truck( options){ this.state = options.state || "used"; this.wheelSize = options.wheelSize || "large"; this.color = options.color || "blue"; } // FactoryExample.js // Define a skeleton vehicle factory function VehicleFactory() {} // Define the prototypes and utilities for this factory // Our default vehicleClass is Car VehicleFactory.prototype.vehicleClass = Car; // Our Factory method for creating new Vehicle instances VehicleFactory.prototype.createVehicle = function ( options ) { switch(options.vehicleType){ case "car": this.vehicleClass = Car; break; case "truck": this.vehicleClass = Truck; // <------------------- THIS LINE /!\ break; //defaults to VehicleFactory.prototype.vehicleClass (Car) } return new this.vehicleClass( options ); }; ```

nitink4472 commented 5 years ago

The default statement under switch can't be under comment (//).

alvarocastro commented 5 years ago

What I meant is that if I don't specify the vehicleType in the options object, I get a new vehicle of the tipe it was last created, and not of the defaulted type, becase in each createVehicle call, the this.vehicleClass is changed (if a type is provided)

vf.createVehicle(); // Returns a Car
vf.createVehicle({ vehicleClass: 'truck'}); // Returns a Truck
vf.createVehicle(); // Returns a Truck

What confuses me is: why change the value of this.vehicleClass and don't use it as a read-only?

Wouldn't this be more clear/predictive and less messy?

VehicleFactory.prototype.createVehicle = function ( options ) {
  let vehicleClass = this.vehicleClass;

  switch(options.vehicleType){
    case "car":
      vehicleClass = Car;
      break;
    case "truck":
      vehicleClass = Truck; // <------------------- THIS LINE /!\
      break;
  }

  return new vehicleClass( options );
};