HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMajor

Basic Java library to hold books

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
booksjavaholdlibrarybasic

Problem

For my education, I have written this basic library in Java. Maybe someone can see something that I can do better, as I'm just beginning with Java and I'm hungry to learn more.

public class Library {
    int Capacity = 10;
    int Volume = 0;
    Book[] storage = new Book[10];

    public Library() {
        System.out.println("Hello, I am a library, which can store up to 10 books!");
        this.storage = new Book[10];
    }

    public void add(Book book) {
        if (this.Volume = this.Capacity) System.out.println("The library is full!");

    }

    public Book search(String title) {
        for (int i = 0; i < this.Volume; i++) {
            if (title.equals(this.storage[i].toString())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return this.storage[i];
            }
        }
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;
    }

}

public class Book {
    String title;

    public Book(String title){
        this.title = title;
        System.out.println("Book " + title + " created.");}

    public String toString(){
        return this.title;
    };
}

Solution

Naming

Java has a Naming Conventions that you currently don't follow.

int Capacity = 10;
int Volume = 0;


You should have : Capacity -> capacity and Volume -> volume. I really suggest you to read the convention when you learn a language. It helps readers to quickly scan your code.

Visibility

Currently, your fields in your class are package private, which mean that every class in your package can access your fields. I recommend to use private for fields in your classes, until you determine that you need something else.

You really want to have the minimum scope for you variable.

Initialization

You're doing your initalization two times. First when you declare Book[] storage = new Book[10]; you create an array of 10 books with it. Next in your constructor, you do: this.storage = new Book[10];. This does the same thing as previously so you should delete one of them.

Magic values

Magic values are constant that are not named but used directly in the code. In your case you have a default size for your library, but it's hard-coded. You could have a : private static final int DEFAULT_SIZE = 10. That would make it make clear what it used for.

Brackets and code block

I prefer to always use bracket in code block, I find it easier to read, less prone to bugs when you change add lines of code and there is less changes when you actually change it (think for source control like Git).

} else if (this.Volume >= this.Capacity) System.out.println("The library is full!");


Should be

} else if (this.Volume >= this.Capacity) {
    System.out.println("The library is full!");
}


For each loops

This is something you really want to use a lot. They exist in a lot of language and will make it easier to read when you loop over a collection and want to access each object. When you don't need to know where in the collection your are (the index), prefer using for-each.

So instead of :

for (int i = 0; i < this.Volume; i++) {
            if (title.equals(this.storage[i].toString())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return this.storage[i];
            }
        }


You would have :

for (Book book : this.storage) {
   if( book == null) {
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;
   }
   if (title.equals(book.toString())) {
      System.out.println("The book with the title " + title + " exists in the library!");
      return book;
   }
}


Someone point me out in comments that there was a bug with my original for-each loop. Now that I fixed it, it is really less elegant. There will be duplication and a new corner case added to the method. This stuff make the method more complicated than not. This is partly because you have an array[] instead of something like and ArrayList (always use List the interface if you use ArrayList). This has already been covered in other answers, so if you change to a more dynamic structure use for-each, if not then continue using your for loop but keep in mind that for-each exist.

Code Snippets

int Capacity = 10;
int Volume = 0;
} else if (this.Volume >= this.Capacity) System.out.println("The library is full!");
} else if (this.Volume >= this.Capacity) {
    System.out.println("The library is full!");
}
for (int i = 0; i < this.Volume; i++) {
            if (title.equals(this.storage[i].toString())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return this.storage[i];
            }
        }
for (Book book : this.storage) {
   if( book == null) {
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;
   }
   if (title.equals(book.toString())) {
      System.out.println("The book with the title " + title + " exists in the library!");
      return book;
   }
}

Context

StackExchange Code Review Q#163073, answer score: 33

Revisions (0)

No revisions yet.