current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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 the only classes I did not really have any questions over. I am aware splitting these classes into two is completely unnecessary in this scenario, but I was just messing around with inheritance and never changed it back.

class Person{
  constructor(name, age, phoneNumber){
    this.name = name;
    this.age = age;
    this.phoneNumber = phoneNumber;
  }
}

class Renter extends Person{
  constructor(name, age, phoneNumber, libraryID){
    super(name, age, phoneNumber)
    this.libraryID = libraryID;
    this.books = [];
  }

  addBook(book){
    this.books.push(book);
  }

  removeBook(id){
    var index = this.getIndex(id);
    this.books.splice(index, 1);
  }

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

Another general question: Am I accessing the class variables "formally"? It does not feel right setting a instance variable without a setter or returning a instance variable without a getter.

share|improve this question
up vote 2 down vote accepted 
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.

share|improve this answer
up vote 0 down vote

I wanted my Library class constructor to be optional, the option to add a single book, multiple books or none at all

Yes this is possible using the arguments object in Javascript

   

 class Library{
      constructor(){
        this.books = Array.prototype.slice.call(arguments);                            
        console.log(this.books);
        console.log(arguments.length);
      }
    }
         var library2 = new Library(new Array("Saab", "45", 2),new Array("Saab2", 56, 7));

Note : Implementing this will require a for(){} to be able to add as much books you require.

// So you can add as much arguments has you need

    var library1 = new Library(new Array("Saab", "45", 2));

    var library2 = new Library(new Array("Saab", "45", 2),new Array("Saab2", 56, 7));

    var library3 = new Library();
  • As you clearly stated this is an OOP structure, then your Library should be taking a BOOK object as an argument has opposed to you creating an Array called this.books to contain all the arguments passed because passing the Book class requires you to fill in title, pageCount, id For instance 
 var library2 = new Library(new Book("Saab", "45", 2),new Book("Saab2", 56, 7));
  • Javascript is a loose language. Note, it's a bit wierd for instance you would expect when you declared available, currentReader and checkOutdate variables the scope was limited to setdefaults() and hence you repeated this variables in the constructor. The truth is the Book class has access to it because you used this. So you can do the below . This will avoid DRY(Dont Repeat Yourself)

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

var book = new Book('Pokemon', 23, 1);
console.log(book.available);

  • Just for clarity sake, I would have this line at the top before calling defining the classes
    var dateFormat = require('dateformat');

As @Joseph the Dreamer suggested your checkIn and checkOut can be improved on as their names differ from their functionalities .
I hope this helps. Cheers

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.