-4

hey this is the function I need to write: int addBookToLibrary(Book book) Adds the given book to this library, if there is place available, and it isn't already in the library. Parameters: book - The book to add to this library. Returns: a non-negative id number for the book if there was a spot and the book was successfully added, or if the book was already in the library; a negative number otherwise.

and my code: (booksarray= list of books object, bookcapacity-the length of the array, bookcounter-the current amount of book in the array)

int addBookToLibrary(Book book) {
    boolean found= false;
    for (int i = 0; i <= bookCapacity; i++) {
        if (booksArray[i]==book){
            found=true;
        }
    }
    if (bookCapacity> bookCounter && !found) {
        booksArray[bookCounter] = book;
        bookCounter++;
    }
    return bookCounter;
}

Please help me understand what the problem is in the code :)

papi
  • 227
  • 1
  • 4
  • 11
  • 5
    ["does not work"](http://importblogkit.com/2015/07/does-not-work/) is not a meaningful problem statement. Please [edit] your question to include the details of *how* your program doesn't work. – azurefrog Mar 23 '18 at 18:30
  • 1
    That break doesn’t make a lot of sense – cpp beginner Mar 23 '18 at 18:30
  • what is the error in the code – humblefoolish Mar 23 '18 at 18:30
  • 4
    Use `Object#equals(Object)` - not `==`. – Elliott Frisch Mar 23 '18 at 18:31
  • @ElliottFrisch I am always amazed how even experienced developers fall for the old '== instead of equals' – Rann Lifshitz Mar 23 '18 at 18:32
  • 3
    *"booksarray= list of books object"* **List** of books? Not **array** of books? A list is not an array. Show us the **declarations** of `booksArray`, `bookCapacity`, and `bookCounter`. Don't just tell us about them. Down-voted for missing relevant code. – Andreas Mar 23 '18 at 18:35
  • @papi : a few recommendations - add a ( && !found) statement to your condition in the for loop - no need to continue iterating once youve met your goals. consider replacing your returned value - the add operation makes more sense returning true for a successful operation and false for a failure – Rann Lifshitz Mar 23 '18 at 18:37

4 Answers4

1

I will give you some hints.

First, I would use Set instead of an array as you are using.

Then, here:

        if (booksArray[i] == book){

This might not work, depending on how you have implemented it. In there you are comparing the reference in booksArray[i] with the reference of book. How do you know when a book is equal another? For example, is it by a field name inside, so when two instances of book have the same name are considered equal?

So, unless you have a requirement for arrays, I would recommend yoy use a Set, implement the equals and hash methods inside the class Book and make the comparison with contains().


EDIT

If you are not allowed to use Set as per your comment, then understand your problem first.

If you do

book book = new Book();

You have created a reference. Then if you put this very same object in the array, you get the same reference inside the array, so when you compare them with ==, they will point to the same object reference and the result will be true.

However, if you obtain the books from somewhere else, or copy the object inside the array (which will create a new Book object, and therefore, with a different reference) then both references will always be different.

In this case, you don't want to compare objects by their reference, but by something else. In that case, (1) you need to implement (override) both equals() and HashCode() methods inside Book. In these methods you specify what makes books equal. For example, books can be equal if they have the same "name" (e.g. book1.name is equal book2.name). Or if they have the same name AND the same author... This depends on your object, it is your decision. And finally (2) you compare books with equals() and not with ==. Here you have an example.

By the way, this is an aside note... To improve your code I would recommend you to do:

int addBookToLibrary(Book book) throws CapacityReachedException {
    if( isLibraryMaxCapacity() ) {
        throw new CapacityReachedException("The library is full");
    }
    if (!contains(book)) {
        addBook(book);
    }
    return numberOfBooks();
}

And then you implement separately the methods isLibraryMaxCapacity(), contains(book), addBook(book) and numberOfBooks(). They are all very simple.

user1156544
  • 1,725
  • 2
  • 25
  • 51
1

This looks a lot like homework but I see that you tried to solve it so I will give help you, other answer suggested using Set which should be used in situations like this but considering that you might be beginner and still didn't look into Java Collections I will try to explain few pitfalls in your code.

First problem is using == for checking equality, while this is great for primitive types, when comparing reference types this isn't enough, in other to properly compare 2 objects you will need to override equals method in your Book class.

This is how it should look like, you didn't provide your book class, but for simplicity this compared book title and author to see if book is the same, you can adjust to your needs.

public class Book {

   private String title;
   private String author;

   @Override
   public boolean equals(Object o) { 
      // Reference equality check(same reference => same object)
      if (this == o) 
        return true;

      // return false if it is null or current object 
      // doesn't have same class as one we are comparing with(they can't be same then)
      if (o == null || getClass() != o.getClass()) 
        return false;

      // Otherwise we know that object belongs to same class so we can cast
      Book book = (Book) o;

      // Compare titles, if they are different we return false
      if(!this.title.equals(book.title))
         return false;

      // Compare authors, if they are different we return false
      if(!this.author.equals(book.author))
       return false;

      // Couldn't find difference so return true
      return true;
   }
}

Another thing which could be done is removing found flag and simply returning once you see that book already exists, so your method should look something like this:

int addBookToLibrary(Book book) {
    for (int i = 0; i <= bookCapacity; i++) {
        // Note that we are using equals here
        if (booksArray[i].equals(book)){
            return -1; // Return -1 so you can check on calling place if book was added or not
        }
    }
    if (bookCapacity> bookCounter) {
        booksArray[bookCounter] = book;
        bookCounter++;
    }
    return bookCounter;
}

And the last problem, that might be intentional but still you should consider it, is that you return bookCounter after you increased it, meaning that you return 1 for element at index 0 inside your array, you could fix this by adding another local variable to keep previous.

int currentIndex = bookCounter;
if (bookCapacity> bookCounter) {
    booksArray[currentIndex] = book;
    bookCounter++;
}
return currentIndex;
FilipRistic
  • 2,661
  • 4
  • 22
  • 31
0

change the below statement in you code

if (booksArray[i]==book)

to

if (booksArray[i].equals(book)
paulophoenix
  • 89
  • 1
  • 10
0

so this is what i would do:

int addBookToLibrary(Book book){
    boolean found = Arrays.asList(booksArray).contains(book);
    if(bookCapacity > bookCounter && !found){
    booksArray[bookCounter] = book;
    bookCounter++;
    }
return bookcounter;
}

the only thing is that i dont know if this solves your problem since you werent really specific about it. just let me know if you need more help

Naugrim00
  • 61
  • 5