4
\$\begingroup\$

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<String, Integer> library) {
    int unfinished = 0;
    if (library.size() < 1) {
      System.out.println("Please add some books to your library.");
    } else {
      for (String book : library.keySet()) {
        if (library.get(book) < 100 && library.get(book) > 0) {
          unfinished++;
        }
      }
      if (unfinished > 0) {
          System.out.println("You have " + unfinished + " unfinished books. They are:");
        }
      for (String book : library.keySet()) {
        if (library.get(book) < 100 && library.get(book) > 0) {
          System.out.println(book);
        }
      }
    }
  }
\$\endgroup\$
3
\$\begingroup\$

Prefer interfaces

public void getIncompleteBooks(HashMap<String, Integer> library) {

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

public void getIncompleteBooks(Map<String, Integer> 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<String, Integer> library = HashMap<String, Integer>();

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) < 100 && library.get(book) > 0) {
          unfinished++;
        }
      }

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

      for (Map.Entry<String, Integer> book : library.entrySet()) {
        if (book.getValue() < 100 && 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<Map.Entry<String, Integer>> getIncompleteBooks(Map<String, Integer> library) {
    List<Map.Entry<String, Integer>> results = new ArrayList<>();

    for (Map.Entry<String, Integer> 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<Map.Entry<String, Integer>> books) {
    for (Map.Entry<String, Integer> book : books) {
        System.out.println(book.getKey());
    }
}

This is the basic display method.

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

    List<Map.Entry<String, Integer>> 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<String, Integer> library) {
    displayTitles(library.entrySet());
}

or

public static double calculateFractionIncomplete(Map<String, Integer> 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow, thank you for the fantastic response! I will go through each of your comments and try to understand everything you have said. Really appreciate you taking the time! \$\endgroup\$ – Raquel Smith Jul 21 '16 at 21:42
  • \$\begingroup\$ Downvoted by mistake (was going to upvote). Would you please edit your answer, so that I can fix that? Sorry for that! \$\endgroup\$ – carlossierra Jul 21 '16 at 23:09

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.