26
\$\begingroup\$

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) {
            this.storage[this.Volume] = book;
            System.out.println("I added the book " + book + "!");
            this.Volume++;
        } else 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;
    };
}
\$\endgroup\$
  • 33
    \$\begingroup\$ Your use of the word "library" is not what developers call a library. It implies that you've written a set of useful classes that can be used by other programmers. What you actually have written is a Java class representing a real-world library. \$\endgroup\$ – Klitos Kyriacou May 11 '17 at 15:56
  • 3
    \$\begingroup\$ Looks quite good. One addition to the other answers: Volume can be quite misleading, since books can be numbered by "volume". A better name would be currentAmountOfBooks. The current prefix is imo very important, since it tells a lot about the variable. \$\endgroup\$ – slowy May 11 '17 at 16:02
  • \$\begingroup\$ Both of your comments could are what I would call answers! Don't hesitate to post and earn rep :) ! \$\endgroup\$ – Marc-Andre May 11 '17 at 17:39
33
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks @Timothy Truckle for the link. I could have use the google one too which is nice too. I was vague intentionally, but a link is fine too. \$\endgroup\$ – Marc-Andre May 11 '17 at 17:03
  • 4
    \$\begingroup\$ The For each part is incorrect. Only the first Volume elements are non-null, so you get an NPE if the book name is not found \$\endgroup\$ – Clashsoft May 12 '17 at 5:08
  • \$\begingroup\$ I don't have the time at the moment to fix the answer, will sure look at it today. \$\endgroup\$ – Marc-Andre May 12 '17 at 13:11
  • 1
    \$\begingroup\$ The last code block still has a bug: If the library is full, you won't print out anything. I'd personally prefer iterating from 0 to volume simply because that is the actual range we want to iterate. Otherwise, check for null and break. \$\endgroup\$ – Nathan Merrill May 13 '17 at 1:32
  • \$\begingroup\$ If the library is full, it will it the rest of the method. I just talked about the loop not the method. Or I don't understand what you said. \$\endgroup\$ – Marc-Andre May 13 '17 at 1:34
11
\$\begingroup\$

toString has special meaning in Java:

In general, the toString method returns a string that "textually represents" this object. The result should be a concise but informative representation that is easy for a person to read.

I would not expect the title of a book to "represent" it - after all, there might be other books with the same title. Instead, the common pattern is to use the name getTitle (a "getter") for fields.


The capacity of the library and the length of the array of books are not linked. You should not store this limitation in two places. If someone changes one of the numbers your code instantly has a bug - either it will attempt to store too many books or it will have spaces left over when reporting that it does not. Instead either tie the length of the array to the actual capacity (new Book[Capacity]) or use a type like ArrayList which doesn't have to be declared with a length.

\$\endgroup\$
  • 3
    \$\begingroup\$ I think OP meant "Library" as a library for books, not the standard sense. \$\endgroup\$ – Tamoghna Chowdhury May 13 '17 at 6:19
  • \$\begingroup\$ @TamoghnaChowdhury Thank you, fixed the drive-by advice. \$\endgroup\$ – l0b0 May 13 '17 at 7:36
9
\$\begingroup\$

java.util.Optional<T>

A short suggestion because no one has suggested it yet. Instead of returning null or throwing an exception when the book you are searching for isn't found, you should use Optional.

public Optional<Book> search(String title) {
    for (int i = 0; i < this.Volume; i++) {
        if (/* matching logic... */) {
            // logging...
            return Optional.of(storage[i]);
        }
    }
    // logging...
    return Optional.empty();
}

My reasoning against returning null is this: It is never 100% clear if a method will return null or not in Java and maybe someone will think this won't so they won't guard against the null condition with if (foundBook != null) { ... }. If you return an Optional, it's an extremely clear signal that the content of the result may or may not be present. They still may handle it wrong but at least you can be more certain they know what's going on.

My reasoning against throwing some exception when the book is not present is this:

  1. If you throw a checked exception every single call of this method will need to be wrapped with try { Book b = search("blah") } catch (SomeException e) { ... }. Checked exceptions are a pain to deal.
  2. If you throw an unchecked exception then you have the same problem of null, maybe people don't realize it could happen. So they don't wrap it in a try-block.
\$\endgroup\$
  • \$\begingroup\$ I didn't know Java had optionals, but I've seen similar stuff in Swift. I like it! :-D Could you provide an example of how to test for this, if you wanted to? \$\endgroup\$ – holroy May 11 '17 at 22:34
  • \$\begingroup\$ Sure but I don't know what you mean. Like check if it's present? \$\endgroup\$ – Captain Man May 12 '17 at 0:18
7
\$\begingroup\$

List

You can replace Volume and storage with a java.util.List.

List storage = new ArrayList();

the List will track its capacity and dynamically expand as needed.

You can still check the maximum amount of books in this way:

if (storage.size() < Capacity) 

Else if

The "else if" can be replaced by a simpler "else". I also do not like the change of indentation there and I'd prefer to see the line split in two.

this

In Java the this keyword is usually used only when there is some kind of ambiguity. In most situations there is no need to use it and, in my opinion, makes the code harder to read.

toString is for debug

The toString() method is intended for debug output and you should not give it a special meaning in your code. If you want to compare the title refer to the title only, if you want to do a general comparison on the book use the equals/hashcode method pair (see here).

System.out

For production code, you should avoid to use System.out and use a logging library instead.

Write like you think

This one is quite subtle: in this line

if (title.equals(this.storage[i].title)) {

I would switch the roles:

if (this.storage[i].title.equals(title)) {

this doesn't make any difference for the compiler but it's a big difference for humans: here I'm examining the books, not the title. I'm looking for a book whose title equals the given title and this "reasoning" should be reflected in the code. This reads even better:

book = storage[i]
if (book.title.equals(title)) {

Files

In general is better to place each class in it's own file (and it's mandatory for public classes, I suppose this is a cut and paste from two files).

Mixing stuff

These three lines are intertwined: you start dealing with the data storage, then suspend and move to logging something for the user, then back again to complete the data update:

this.storage[this.Volume] = book;
System.out.println("I added the book " + book + "!");
this.Volume++;

This mixing makes much easier to make mistakes, the code is harder to read and refactoring much harder (like moving some related lines in a separated method).

Hidden behaviour

The add method is bad: there is no (reasonable) way fro the callet to know if the book was added or not. In these situations you should return a boolean value to indicate whether it succeded or not.

null vs Result

There is nothing wrong in returning null but this can be improved. An exception here is inappropriate as I think it's normal, not exceptional, to look for a missing book. What you can use here is a SearchResult class.

class SearchResult {
    boolean found;
    String reason;
}

the big advantage is that you can better keep track of the failed searched, setting the reason to the provided title. In this example would clearly be over-engineering but in many situations it's a good option.

Code formatting

Code formatting is inconsistent: do not do formatting by hand, choose a good editor (IntellJ, Eclipse, etc.) and let it reformat the code for you every time you save.

Log ASAP

In the Book constructor:

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

I would place the println at the beginning: in general it's better to log the passed data before you start to process it so if something fails the log helps you. In the case the processing is minimal but is't a good habit to get. In the case you would write something a little different like "Creating book...".

\$\endgroup\$
  • \$\begingroup\$ Instead of some custom SearchResult, why not use the builtin java.util.Optional<T> class? \$\endgroup\$ – Captain Man May 11 '17 at 20:34
  • 1
    \$\begingroup\$ Optional does not allow to add any extra information. It just allows to delay/relocate the "null check" in a standard way and to make it explicit that "null" is a possible result. But if later you want to know why the search failed (to debug, to log failed searches, etc.) Optional does not add much. \$\endgroup\$ – lorenzo May 11 '17 at 21:01
  • \$\begingroup\$ That's all true, but none of that extra work is needed for something so simple. If the class had a list of books or something checked out then I would agree, because you could say it's been checked out or it's not carried. \$\endgroup\$ – Captain Man May 12 '17 at 0:17
  • \$\begingroup\$ If you're going to replace the array with a List<Book> it would be even better to replace it with a Map<String, Book> from title to book, otherwise your search method is going to do an unnecessary O(n) search. \$\endgroup\$ – David Conrad May 12 '17 at 3:56
  • \$\begingroup\$ @CaptainMan Of course this is a simple exercise so about everything is overkill. But the input string(s) alone may be enough to be kept around. Suppose you are looking for a book by title and author using two regular expressions and these are provided dynamically from a GUI. I've found it useful for database queries: "No results for entity Xy with search params [...]". Maybe the input string was "corrupted" somewhere, etc. \$\endgroup\$ – lorenzo May 12 '17 at 7:26
4
\$\begingroup\$

Here are made some changes:

  1. Removed reinitialization from the constructor, because you already initializing.
  2. Removed if (this.Volume >= this.Capacity) this is already held by else property.
  3. Changed you iterative cycle into the foreach cycle, it more readable and easier to use.
  4. Removed ; after }
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!");
    }
    public void add(Book book) {
        if (this.Volume < this.Capacity) {
            this.storage[this.Volume] = book;
            System.out.println("I added the book " + book + "!");
            this.Volume++;
        } else System.out.println("The library is full!");

    }

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

class Book {
    String title;

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

    public String toString() {
        return this.title;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Nice edit! Thanks for improving your answer! \$\endgroup\$ – Marc-Andre May 11 '17 at 15:08
4
\$\begingroup\$

As already pointed out, using a dynamic data structure is better, but this is an exercise anyway, so let us leave that consideration aside and accept the static solution with the predefined size.

int Capacity = 10;
Book[] storage = new Book[10];

Besides the naming, it is not good to use 10 at two places. You might want to change the first occurrence one day, and forget to change the other.

final static int capacity = 10;
Book[] storage = new Book[capacity];
\$\endgroup\$
  • \$\begingroup\$ As a constant, capacity should be in UPPERCASE. \$\endgroup\$ – user131519 May 29 '17 at 8:56
4
\$\begingroup\$

I'm going to repeat some of what others said and add a few things which are very important to learn and no one mentioned them (for known reasons).

Naming Conventions

You should use the naming conventions defined in the Java Language Specifications, JLS 6.1. Declarations (starting from the small text "Naming Conventions"). The JLS is something you will want to familiarize with over time, although it's fairly technical, so don't worry much about it right now.

You will want to rename to lowercase capacity and volume.

Use Declared Values

If you intended to use the number held in capacity, use the field reference instead. Then you only need to change the number in one place and it's also clearer where the numbers in the code come from (magic numbers).

You're using it in the array initialization (which you do twice) and in the print statement.

Constants vs. Variables

Determine which of the values you declare are constant and which can change. Those which can't change should be declared final. Consider setting these constants in the constructor if you want to allow a different value for each instance.

Is capacity something that is constant for each Library instance? Is title constant for each Book? If so, declare them final. Maybe request the capacity value in the constructor like you did for title.

Encapsulate

Use visibility modifiers to control access to an object's fields and methods. This allows each object to control its behavior and expose its services. Search online for the benefits of encapsulation, like here.

Looks like all the fields should be private. If you want someone from outside the class to be able to read and/or write them, use getters and/or setters.

Override Object's Methods

This is very important! In fact, Brian Goetz (Java architect) talked about this recently, saying how developers don't do this, why, and why they should.

You did well overriding Book's toString. You will really want to override its equals and hashCode as well. Your IDE has tools to generate these methods for you. You can also look at Project Lombok that help to reduce boilerplate code.

It is extremely recommended that you use the @Override annotation when overriding methods.

Usage of this

Generally you don't want to use this whenever possible, but to you use it as least as possible. Opinions vary on this one (no pun intended).

return this.title; could be return title;, which is more readable. Same for Library usages.

Flow Control

When using if and else, check the logic to see what is the best way to specify your flow.

You are checking if (volume < capacity) and then else if (volume >= capacity), but if the first check fails, then the second must succeed, so no need for it - a simple else will do.

"foreach" Loop

When iterating over elements where the index is not important, you should use the "foreach" (or "enhanced for") loop. Search online for the full comparison, like here.

for (int i = 0; i < volume; i++) could be for (Book book : storage).

Beware the equals NPE Trap

The equals method my IDE generated checks for nulls, but familiarize yourself with the Objects class and its methods, in this case the static equals methods, which checks for nulls for you. Note that o1.equals(o2) will throw a NPE if o1 == null, but not if o2 == null, which is where the above method comes in handy.

If someone passes a null String to your search method, you will get a NPE.

Consider Appropriate Return Types

What return type suits an operation the most?

Your add method could possibly return a boolean to specify if the book was added or not. Java collections do this as well (see below).

Consider Using Optional<T>

... but don't overdo it. This was a topic for another talk by the Java architects, this time by Stuart Marks. I won't go into all of this, but beware when returning null. What you have currently is perfectly acceptable.

Your search can return and Optional<Book>, but I wouldn't bother with it. Document your method and say that it returns null if no match is found.

Consider Using Collections

Java's collection offer a lot of benefits over arrays. Again, huge topic, search online.

If you can use a List<Book> instead of your array, you could benefit from not needing the volume field at all - instead you have list.size(). This allows to simplify your add method. List also returns a boolean for its add method in case you want to use that as a return value.

Consider Using Streams

Java 8's streams allow to perform bulk operation easily and they are more readable. Search online for advantages of streams over loops.

Your for/"foreach" loop could be replaced with a stream that does the search and finds results in one go. See code below.

Nitpick

You don't need a ; at the end of a method declaration.

The toString of Book has that.


Code time!

Book:

public class Book {

    private final String title;

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

    public String getTitle() {
        return title;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((title == null) ? 0 : title.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        Book other = (Book) obj;
        if (title == null) {
            if (other.title != null)
                return false;
        } else if (!title.equals(other.title))
            return false;
        return true;
    }

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

Library with arrays (you can play with List yourself):

public class Library {

    private final int capacity;
    private int volume = 0;
    private Book[] storage;

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

    public void add(Book book) {
        if (volume < capacity) {
            storage[volume] = book;
            System.out.println("I added the book " + book + "!");
            volume++;
        } else
            System.out.println("The library is full!");
    }

    public Book search(String title) {
        for (Book book : storage) {
            if (Objects.equals(title, book.getTitle())) {
                System.out.println("The book with the title " + title + " exists in the library!");
                return book;
            }
        }
        System.out.println("The book with the title " + title + " does not exist in the library!");
        return null;

        //or with streams: returns an Optional<Book>
        return Arrays.stream(storage)
            .filter(book -> Objects.equals(title, book.getTitle()))
            .findAny();
    }
}
\$\endgroup\$
3
\$\begingroup\$

Thanks for sharing your code.

Here are some additional thoughts of mine:

Object Oriented Programming

Java is an object oriented programming language. But using Java does not yet mean you're doing OOP.

OOP means that you follow certain principles which are (amongst others):

  • information hiding / encapsulation
  • single responsibility / separation of concerns
  • same level of abstraction
  • Dependency Injection / Inversion of Control
  • KISS (Keep it simple (and) stupid.)
  • DRY (Don't repeat yourself.)
  • Program against Interfaces (not implementations)
  • "Tell! Don't ask."
  • Law of Demeter ("Don't talk to strangers!")
  • favor polymorphism over branching

Most of this principles do not apply to your small program. But I think you should get familiar to these terms as soon as possible.

On the other hand there are some violations of that principles in your code. Some have already been pointed out by @Marc-Andre.

Here is some more

single responsibility / separation of concerns

Each method of your class should have exactly one well defined responsibility.

E.g. a constructors responsibility is always to initialize the (final) object variables. (Variables which do not change their content during the lifetime of the object should have the final keyword.)

It does this usually by assigning its parameters to those object variables (as you did in the Book class). Therefore the constructor should not do any output. Sometimes it is useful when looking for bugs but the output lines should be removed from the constructor as soon as possible.

same level of abstraction

A method can either to primitive operations on variables (assignments, access, calculations) or call methods (in the same class or on objects of other classes aka dependencies). When doing the latter it is called a composed method.

Your methods should do either one. Eg.: your method search should be changed to something like this:

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

Don't be afraid of many small methods. They give you the possibility to move them to other (new) classes as your current class grows to big...

Avoid returning null

Null References, the Billion Dollar Mistake

There are a few cases where null is a valid element of the result set, but most cases I've seen null is (miss-)used as an error signal.

Instead of returning null you should throw an Exception. For this purpose you might create a custom excception like BookNotInLibraryException:

public class BookNotInLibraryException extends Exception{
    public BookNotInLibraryException(String bookTitle){
       super("There is no book with title >" + bookTitle + "< in the Library!");
    }
}


public Book search(String title) thows BookNotInLibraryException {
    for (int i = 0; i < this.Volume; i++) {
        Book currentBook = getBookAt(i);
        if (title.equals(currentBook.toString())) {
            reportFound(title);
            return currentBook;
        }
    }
    throw new BookNotInLibraryException (title);
}    

This would also change place where you call this method:

 // somewhere
 Library lib = new  Library();
 try{
   Book bookFromLibrary = lib.search("Some Book Title");
   // do something with the book, it is never null here
 } catch (BookNotInLibraryException e){
   System.err.println("error processing boook: "+e);
 }
\$\endgroup\$
  • 1
    \$\begingroup\$ Java 8 introduces the Optional type, which is a cleaner alternative to null. The advantage is that it forces callers to deal with the case when there is no result. With exceptions this is enforced too, but try/catch is very intrusive to control flow. Exceptions should be reserved for exceptional cases, i.e., errors, not for representing part of the result set. \$\endgroup\$ – MauganRa May 11 '17 at 19:42
  • 1
    \$\begingroup\$ "Exceptions should be reserved for exceptional cases" A book missing in the lib is an exceptional case IMHO... \$\endgroup\$ – Timothy Truckle May 11 '17 at 19:52
  • 5
    \$\begingroup\$ @TimothyTruckle I think most people would disagree with your opinion on that one \$\endgroup\$ – Captain Man May 11 '17 at 20:36
  • 3
    \$\begingroup\$ Throwing a NullPointerException for a failed search is a terrible idea and violates the principle of least astonishment and the contract of NPE which reads, "Thrown when an application attempts to use null in a case where an object is required." You could throw an IllegalArgumentException or a NoSuchElementException but that is just going to make your API difficult to use. \$\endgroup\$ – David Conrad May 12 '17 at 4:00
  • 1
    \$\begingroup\$ So would you change your example code to reflect that? \$\endgroup\$ – Tamoghna Chowdhury May 13 '17 at 6:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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