Library system manager

I'm new to JavaScript OOP and still playing around with it. I have put together a few specific questions over specific details throughout the code that are bothering me.

I wanted my Library class constructor to be optional, the option to add a single book, multiple books or none at all, when creating the object. Is this a good way of achieving this goal?

class Library{
  constructor(items){
    this.books = [];
    if(typeof items != 'undefined' && items instanceof Array){
      this.books.push.apply(this.books, items); // Concat two arrays
    } else if(typeof items != 'undefined') {
      this.books.push(items); // Add one element
    }
  }

  checkIn(id, renter){
    var index = this.getIndex(id);
    if(!this.books[index].available){
      this.books[index].setDefaults();
      renter.removeBook(id);
    } else {
      throw "Book Already Checked In";
    }
  }

  checkOut(id, renter){
    var index = this.getIndex(id);
    if(this.books[index].available){
      this.books[index].available = false;
      this.books[index].currentRenter = renter.name;

      // Adds current date to check out information.
      var dateFormat = require('dateformat');
      this.books[index].checkOutDate = dateFormat(new Date(),  "dddd, mmmm dS, yyyy, h:MM:ss TT");

      renter.addBook(this.books[index]); // Adds book to renter's inventory.
    } else {
      throw "Book Not Available";
    }
  }

  getIndex(id){
    for(var i = 0; i < this.books.length; i++){
      if(this.books[i].id == id) return i;
    }
    return -1;
  }
}

My Book class works, but just does not feel correct at all and looks very sloppy. Any thoughts?

class Book{
  constructor(title, pageCount, id){
    this.title = title;
    this.pageCount = pageCount;
    this.id = id;
    this.available = true;
    this.currentRenter = null;
    this.checkOutDate = null;
  }

  setDefaults(){
    this.available = true;
    this.currentRenter = null;
    this.checkOutDate = null;
  }
}

These are t

checkIn(id, renter){...}
checkOut(id, renter){...}

You're not actually checking in the id of a book. You're checking in a book. So I would suggest accepting an instance of a book and get the ID from it instead of passing in an ID. You're already doing it for renter, passing in the renter instance, so it shouldn't be different.

this.books = [];

I would suggest using an object {} instead of an array. One problem with using an array, and the ID as index is that you unknowingly increase the length of the array. If you check the length of an empty array after doing this.books[100] = new Book(...), you'll see that it's 101. The array stretched to 101 elements and left gaps in between.

If the book is checked out, you can either null its entry in the object or delete it altogether with delete. It also makes getIndex unnecessary as you can simply access the object directly with id as key.

throw "Book Already Checked In";

I would suggest throwing an instance of Error instead of a string. Error-catching code will most likely expect an instance of Error and not a string. You also lose out on extra information that comes with an instance of Error. Lastly, not OOP-ish. :P

checkOut(id, renter){
  ...
  var dateFormat = require('dateformat');

Dependencies should be done outside anything. Behavior depends on the loader used. If you used RequireJS this would try to resolve dateformat, and load it on the fly. Other module loaders may inline the code, causing this function to bloat.

this.currentRenter = null;
this.checkOutDate = null;

The book shouldn't be aware of its renter. All it should be describing is the book itself.

If you want something like a library card, then I suggest making a property libraryCard that holds an array of LibraryCardRecord objects, each containing the date of checkout and the id of the renter at that date.


It does not feel right setting a instance variable without a setter or returning a instance variable without a getter.

It's overkill, but it's a good practice to always use a getter and setter. While it's meaningless in the world of all-public JavaScript, it allows you to do intermediate operations before setting or getting. ES6 does have special syntax for getters and setters.

StackExchange Code Review Q#136468, answer score: 2