Book constructor pattern

Question

I am learning how to do object-orientated programming in JS. In particular, I am learning about the constructor pattern.

In an attempt to exercise what I have been learning, I made this:

var assert = require("assert");

var Book = (function() {
  // private static variable (why this exhibits static behaviour, I have no clue)
  var books = [];

  var constructor = function(doc) {
    // guard clauses
    assert.ok(doc.title, "title cannot be undefined");
    assert.ok(doc.author, "author cannot be undefined");
    assert.ok(doc.genre, "genre cannot be undefined");

    // public properties
    this.title = doc.title;
    this.author = doc.author;
    this.genre = doc.genre;
    this.read = doc.read || false;
    this.id = books.length;
    this.description = this.title + " is a " + this.genre + " by " + this.author;

    // public function
    this.save = function() {
      books.push(this);
    };
  };

  // public static functions
  constructor.findById = function(id, callback) {
    function cloneBook(book) {
      return {
        id: book.id,
        title: book.title,
        author: book.author,
        genre: book.genre,
        read: book.read
      };
    }
    var book = books.filter(function(book) {
      return book.id === id;
    })[0];
    var clone = cloneBook(book);
    clone.save = function() {
      book.title = this.title;
      book.author = this.author;
      book.genre = this.genre;
      book.read = this.read;
    };
    callback(clone);
  };

  constructor.find = function(conditions, callback) {
    function areNoConditions(obj) {
      return Object.keys(obj).length === 0;
    }
    if (areNoConditions(conditions)) {
      callback(books);
      return;
    }
    var foundBooks = [];
    for (var prop in conditions) {
      books.forEach(function(book) {
        if (book[prop] === conditions[prop]) {
          foundBooks.push(book);
        }
      });
      callback(foundBooks);
    }
  };

  // return the function (the constructor)
  return constructor;
}());

...

var book = new Book("The Great Gatsby", "F. Scott Fitzgerald", "Novel");
book.save();
var books = Book.find();
book.read = false;
book.save();
console.log(books);

My aim was to emulate the Mongoose Model API.

My particular concerns are:

1. Is the constructor pattern the most applicable pattern here?
2. Is my code idiomatic?

I have decorated my code with comments for the purposes of this answer so you can know what I am thinking. I come from a C# background.

Answer 1

It looks ok at first glance (proper use of camelCase and PascalCase, proper indentation, etc.), but there are some issues when you dig a little deeper.

• You have a bug: Because you define a book's ID as simply books.length, and because you set the ID in the constructor, you're assuming/relying on every book instance to either be added ("saved") immediately after instantiation or discarded forever.

  If I were to do this:

  book1 = new Book({ ... });
  book2 = new Book({ ... });
  

  I get two books with the same ID. Of course, id is just another property, but in this case, I think it's safe to assume that it's supposed to be a unique ID.

  For now, you could simply move the assignment of a book's ID to the save method, so it only gets assigned there.

  However, if you ever add the ability to delete books, you'll run into trouble again, since removing something from the array would change its length, and you'd end up with multiple books sharing the same ID.

  Perhaps the simplest thing would be to declare yet another closure variable (aka a private, static variable) simply called nextId or something. Then, whenever a book is saved, give it that value as its ID, and increment nextId. Same basic principle as auto-incrementing IDs in a database.

  Of course there's still the caveat that anyone can just change a book's id property to "monkey" after the fact, but one step at a time.

• How can anyone change the ID after the fact even with the extra precautions I mentioned? Well, if I call Book.find with no conditions, I get a reference to an otherwise private variable: books. Then I can do whatever I want with its contents. If I call it with some conditions, I don't get the books array itself, which is good, but I still get some of the same book objects that books holds. So again, I'm free to do what I want.

  More simply, though, I can just alter the instance I created after it was saved since save just does books.push(this). So I can do this:

  var book = new Book({title: "War and Peace", author: "Leo Tolstoy", genre: "novel"});
  book.save();
  book.id = "monkey"; // cue mischievous snickering
  

  The book instance I changed after saving is the same instance the books array holds a reference to, so the damage is done.

  Of course, right now, I could also just set book.id before saving, and achieve the same, since save doesn't care what it's stuffing in the array.

  Also, if I do this:

  var book = new Book({title: "War and Peace", author: "Leo Tolstoy", genre: "novel"});
  book.id = "monkey";
  book.save();
  book.save();
  book.save();
  

  I end up with three identical books in the books array, all of which have the ID "monkey". It's not even that it's three copies; it's the same exact object appearing three times. Hmm....

• You have another bug: Book.findById assumes it always finds something. When it doesn't, the script breaks because it calls cloneBook(undefined).

• The description is fixed once defined, so changing, say, the title doesn't update the description. Description would be better off as a method.

• The assert module isn't really necessary. I'd rather see the constructor throw a more specific exception of some kind (you can throw anything in JavaScript, so the "exception" can just be a string to start with). Also, your message says "cannot be undefined", but assert.ok just checks for general thruthiness, not undefined specifically. So it'll balk at false, 0, NaN, "", and null as well as undefined. Most of those are fine to complain about in this case, except maybe the empty string (maybe you don't want to put a genre in there, but still want a book instance). Besides, checking for thruthiness still doesn't check for type. So the title could be an array, the genre could be a Date object, author could be Number.NEGATIVE_INFINITY etc... However, that's simply the nature of dynamic typing, so it's up to you how far you want to go.

  Still, it might be more beneficial to move the validity checks to the save method. Right now, you can't make a "blank" book by saying new Book; you have to know most of its details ahead of time.

  ... which begs the question: If you have all the details in an object already, do you need to make a Book out of em? Eh, this is an exercise, so I'll just go with it.

• Why the callbacks? None of what you're doing is asynchronous, and JavaScript is single-threaded. So you can just return. It'd make the API a lot simpler. I suppose it's because you're mimicking Mongoose and its functions are asynchronous, but still. It's a little too cargo cult'y. (Note that using callbacks allows for really neat stuff, and learning to work with asynchronous JavaScript is a worthy goal; in this case, however, it complicates matters.)

• The idea of returning a clone from findById to keep some properties "safe" is fine in and of itself. However, because you've hardcoded which properties are editable, it's not great for maintainability.

• Why bother with the areNoConditions function? For one, it's a really odd name, but it also just wraps a single condition you might as well do inline.

• Why not use filter in find?

Long story short, there are some issues here, but for a learning exercise it's not too shabby! You just have to decide how far you want to go with it, because a large portion of this seems to be about controlling access to properties. But that's kinda hard to do really well in JavaScript. Mongoose is forced to do all those things, but it's not the "path of least resistance" in JavaScript. Hence, it may not be the best thing to mimic.

You can "sort of" do strictly typed, access-controlled stuff in JavaScript. But unless you really need to, why bother? It's going against the grain, and often just encourages headaches.

By the way:

// private static variable (why this exhibits static behaviour, I have no clue)

Because it's a closure. All the functions defined while it's in-scope will retain a reference to it. So while you return the constructor function, that function still has access to books.