Prints Books from a HashMap that are complete or incomplete

This code is meant to display book titles that are incomplete. I have a HashMap with the book title and a number that represents the amount of the book that has been read.

I want the program to:

• Check to see if there are any books in the HashMap

• Check to see if there are any started, yet incomplete books

• If there are any Started, yet incomplete books, it will print "You have x incomplete books. They are:"

• Finally, it will print out a list of incomplete books

The code works, but I feel like it probably has too many loops and could be cleaned up a lot.

import java.util.HashMap;

public class Library {
  public Library () {

  }

public void getIncompleteBooks(HashMap library) {
    int unfinished = 0;
    if (library.size()  0) {
          unfinished++;
        }
      }
      if (unfinished > 0) {
          System.out.println("You have " + unfinished + " unfinished books. They are:");
        }
      for (String book : library.keySet()) {
        if (library.get(book)  0) {
          System.out.println(book);
        }
      }
    }
  }

Prefer interfaces

public void getIncompleteBooks(HashMap library) {

As a general rule, you should favor interfaces over implementations as types.

public void getIncompleteBooks(Map library) {

Now you can change to another Map implementation without changing this method at all.

Object fields

And of course, it would be more common to make library an object field.

private Map library = HashMap();

Then you wouldn't have to pass it as a parameter to objects of the class.

public void getIncompleteBooks() {

If you don't do that, you should declare the methods static so people don't expect them to use data from the object.

entrySet()

for (String book : library.keySet()) {
        if (library.get(book)  0) {
          unfinished++;
        }
      }

While it will work, a keySet is the wrong tool here. Consider

for (Map.Entry book : library.entrySet()) {
        if (book.getValue()  0) {
          unfinished++;
        }
      }

Now you aren't doing a hash operation just to iterate over the collection.

Separate logic and display

You mix your data operations with your display operations. This can be fragile and hard to maintain, as you end up with many similar methods. Consider how else you could write this.

public static List> getIncompleteBooks(Map library) {
    List> results = new ArrayList<>();

    for (Map.Entry book : library.entrySet()) {
        int percentFinished =  book.getValue();
        if (0 < percentFinished && percentFinished < 100) {
            results.add(book);
        }
    }

    return results;
}

I made this static because it uses none of the object fields of the class, just the parameters.

Adding the percentFinished variable does two things here. One, it means that we only call the getValue() function once. The compiler probably does that anyway. Two, it makes clear what the value actual means. This is what is called "self-commenting" code. It replaces a comment that says, // getValue returns the percent of the book that is finished.

I switched to four column indent because that's more common in Java code.

This is what I would expect a getIncompleteBooks method to do. It just filters out the books that you don't want. You can then use it in multiple ways.

public static void displayTitles(Collection> books) {
    for (Map.Entry book : books) {
        System.out.println(book.getKey());
    }
}

This is the basic display method.

public static void displayTitlesOfIncompleteBooks(Map library) {
    if (library.isEmpty()) {
        System.out.println("Please add some books to your library.");
        return;
    }

    List> incompleteBooks = getIncompleteBooks(library);

    if (incompleteBooks.isEmpty()) {
        return;
    }

    System.out.println("You have " + incompleteBooks.size() + " unfinished books. They are:");
    displayTitles(incompleteBooks);
}

This should produce the same output as your original method.

Notice that it uses early return on exceptional conditions like an empty library or no unfinished books. This avoids heavy nesting.

I used isEmpty rather than comparing the size to 0. Functionally I can't think of a way that this will matter, but I find this version easier to read.

While this is more code for this specific problem, this allows you to solve other problems easily. For example,

public static void displayTitles(Map library) {
    displayTitles(library.entrySet());
}

or

public static double calculateFractionIncomplete(Map library) {
    if (library.isEmpty()) {
        // consider throwing an exception instead
        System.out.println("Please add some books to your library.");
        return 0.0;
    }

    return getIncompleteBooks(library).size() / (double) library.size();
}

I cast the size to double so that Java wouldn't use integer arithmetic and truncate the result.

StackExchange Code Review Q#135429, answer score: 3