Personal Library complete code with the UnFinished books addition


#1

hey, i would like for some notes for improve my coding.

import java.util.HashMap;

public class Library {

  public Library(){
	    
  }
    public void getFinishedBooks( HashMap<String, Boolean> library) {
      if (library.size() < 1) {
        System.out.println("Error! Hashmap is Empty.");
      } else {
        for ( String book: library.keySet()){
         
          if (library.get(book) == true) {
            System.out.println("This book has been completed " + book);
          }
        }
        }
      }
    
    public void getUnFinishedBooks (HashMap<String, Boolean> library) {
    	if (library.size() < 1) {
    		System.out.println("Not Good Capara");
    	}else {
    		for (String book: library.keySet()){
    			if (library.get(book) != true){
    				System.out.println("This book is not completed: " + book);
    			}
    		}
    	}
    }
    
    
    
   public static void main(String[] args) {
	   
     HashMap<String, Boolean> myBooks = new HashMap<String, Boolean>();
     myBooks.put("Road Down The Funnel", true);
     myBooks.put("Rat: A Biology", false);
     myBooks.put("TimeIn", true);
     myBooks.put("3D Food Printing", false);
     
     Library myLibrary = new Library();
     myLibrary.getFinishedBooks(myBooks);
     
     myLibrary.getUnFinishedBooks(myBooks);
    }

}


#2

What someone reading this will first notice is inconsistent formatting. You use both tabs and spaces, and the lowest amount of spaces used is 0 and the highest is 6. I suggest one of: 2 spaces, 4 spaces, tabs.

missing space on the left

missing space before {, superfluous to test if a boolean is true, use the boolean directly

use isEmpty method

Can omit what's inside <>, java will infer it from the variable's type

Skip the constructor if it doesn't do anything, and space before {

That's a whole lot of newlines that doesn't exist between other methods.

After some cleaning up:

I also changed if-else to use return instead, because I don't like nesting where it can be avoided. Some would argue the opposite, that functions should have a single exit point. It's a trade-off either way. I think the bigger the method is, the more one would want a single exit point.

With formatting out of the way one could then start thinking about what's actually being done. Methods with names starting with "get" are suggesting that they return something, not that they print something. Perhaps they should be renamed from get to show, or perhaps the printing should happen somewhere else.


#3

thank you a lot ionatan. i've learned a lot.


#4

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.