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.

• anon: "I have decorated my code with comments for the purposes of this answer" That's a stupid idea. You should always have comments explaining why your code is what it is. Incidentally, note that I said why it is what it is, not just what it is. Your comments should explain why something was written like it is. We can see that return constructor "return[s] the function (the constructor)". You don't need to tell us that in a comment.
• Angular noob: I commented that for completeness, just like I did the guard clauses. The most important things for me to communicate are the intended member type, intended accessibility and whether or not the member is an instance or function member. Now, if I intended to do X but I really did Y, someone can point it out and help me improve the correctness of my code. Ideally, my code would be self-documenting. In a language like Java, I would use accessibility modifiers to communicate the accessibility. Being new to JS, I am not yet sure about the best way to do this, JSDoc comments maybe?

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.

• Angular noob: Thank you so much for your answer - you raise some sound issues that I am going to rectify now. It is tricky, you know, because I come from a strong-typing packground where I strive to adhere to certain principles. My inclination is to apply the same principles to JS but of course, being a dynamic language, the paradigm is totally different. Meh. I think I will get used to it in time. And thank you for your aside on the "private static variable". I was reading some of Douglas Crockford's stuff about closures earlier and all of a sudden, it clicked. Your comment helps re-enforce that, though.
• Flambino: @Angularnoob Happy it helped! And, yeah, the paradigm shift is real. Both dynamic and strong typing have advantages, both have hassles; they're just different ones. But, as you say, you'll get used to it. Besides, it's not like stuff constantly changes type in JavaScript; programmers still like logic. In regular code I of course wouldn't set a book's ID to "monkey" just because I can, but in my answer I was just trying to point out how JS lets me do that, and how hard it is to prevent. Embrace the flexibility it offers instead, and you can do stuff a typed language would hate you for :)
• Flambino: @Angularnoob By the way, points awarded for using the strict equals ===. Despite, or rather because, of the dynamic nature of JS, it's good to know when to use the regular equals, and when to use the strict one. I tend to use strict-equals by default, and only use the regular one "with good reason", so to speak. It's a subtle but neat way of enforcing some rules in your code.