Java dependency injection and hiding details of methods in "Database"-class

Question

I recently posted a bunch of code of my to-do list application and after getting some very helpful and good suggestions on how to improve, I took a shot at it! Here's a link to the former post I made -> A to-do list application written with Java

I have now used the one and the same instance of storage in each of the classes that need it, instead of creating new instances for each class. Namely, the Login-class and the UI-class. I've also created two interfaces and defined methods to deal with the Storage-class. Also I adjusted some of the Storage-classes methods and added new ones, in order to hide the details of the storage-operations from the Login and UI-classes.

I think I nailed it with the dependency injection by passing that one instance of Storage on the constructors of Login and UI-classes. In the StorageInstance-class, I have a method which returns that instance of a storage. Is this the correct way to go about the dependency injection? Namely, that I use the method returnStorage() to do storage-related actions?

As for the interfaces, I wanted to hide the details of what happens in the Storage-class so the UI and Login wouldn't have any idea what is going on under the surface, except that something is happening that relates to the storage-class. Is this the correct way to go about it? Any and all improvements and help are welcome!

I will not post the whole application code in here, only the classes I've changed. The whole project can be viewed from the link above.

public class Storage {

    private HashMap<String, ToDoList> toDoLists;
    private HashMap<String, User> map;
    private File UsernamesAndPasswords;
    private File UsersToDoLists;
    private User user;

    Storage() {
        this.UsernamesAndPasswords = new File("UsernamesAndPasswords.ser");
        this.UsersToDoLists = new File("ToDoLists.ser");
        loadUserNamesAndPasswords(UsernamesAndPasswords);
        loadUsersToDoLists(UsersToDoLists);
    }

    public void saveUsersToDoLists() {
        try {
            FileOutputStream fosTwo = new FileOutputStream(UsersToDoLists);
            ObjectOutputStream oosTwo = new ObjectOutputStream(fosTwo);

            oosTwo.writeObject(this.toDoLists);
            oosTwo.flush();
            oosTwo.close();
            fosTwo.close();
        } catch (IOException e) {
            System.out.println("Exception happened. saveUsersList");
        }
    }

    public void loadUsersToDoLists(File file) {
        if (file.length() == 0) {
            toDoLists = new HashMap<>();
            this.saveUsersToDoLists();
        }
        try {
            FileInputStream fisTwo = new FileInputStream(UsersToDoLists);
            ObjectInputStream oisTwo = new ObjectInputStream(fisTwo);

            toDoLists = (HashMap<String, ToDoList>) oisTwo.readObject();
            oisTwo.close();
            fisTwo.close();
        } catch (Exception e) {
            System.out.println("Exception happened. loadUsersList");
        }
    }

    public void saveUserNamesAndPasswords(HashMap<String, User> loginInfo) {
        try {
            FileOutputStream fos = new FileOutputStream(UsernamesAndPasswords);
            ObjectOutputStream oos = new ObjectOutputStream(fos);

            oos.writeObject(this.map);
            oos.flush();
            oos.close();
            fos.close();
        } catch (IOException e) {
            System.out.println("Exception happened. saveUsernames");
        }
    }

    public void loadUserNamesAndPasswords(File file) {
        //If the file is empty then this method creates a new empty hashmap and saves it
        //in the file
        if (file.length() == 0) {
            map = new HashMap<>();
            this.saveUserNamesAndPasswords(map);
        }
        try {
            FileInputStream fis = new FileInputStream(UsernamesAndPasswords);
            ObjectInputStream ois = new ObjectInputStream(fis);

            map = (HashMap<String, User>) ois.readObject();
            ois.close();
            fis.close();
        } catch (Exception e) {
            System.out.println("Exception happened. loadUserNames");
        }
    }

    public HashMap<String, User> getUserNamesAndPasswords() {
        return this.map;
    }

    public File getUsernamesAndPasswordsFile() {
        return this.UsernamesAndPasswords;
    }

    public HashMap<String, ToDoList> getToDoLists() {
        return this.toDoLists;
    }

    public File getUsersToDoListsFile() {
        return this.UsersToDoLists;
    }

    public void createUser(String userName, String firstName, String lastName, String password) {
        this.user = new User(firstName, lastName, password);
        this.getUserNamesAndPasswords().putIfAbsent(userName, user);
        this.saveUserNamesAndPasswords(this.getUserNamesAndPasswords());
        this.getToDoLists().putIfAbsent(userName, this.user.getUsersToDoList());
        this.saveUsersToDoLists();
    }

    public void validateUsername(String username, String password) {
        if (this.getUserNamesAndPasswords().get(username).passwordEquals(password)) {
            this.user = getUserNamesAndPasswords().get(username);
            this.user.setList(this.getToDoLists().get(username));
        }
    }

    public User returnUser() {
        return this.user;
    }
}

public class UI implements InterfaceUI {

    private final Scanner reader;
    private final Storage storage;
    private Login login;
    private User user;

    public UI(StorageInstance si) {
        this.reader = new Scanner(System.in);
        this.storage = si.getStorage();
        this.login = new Login(si);
    }

    public void start() {
        System.out.println("Login or register");
        String fromUser = reader.nextLine().trim();
        if (fromUser.equalsIgnoreCase("register")) {
            System.out.print("Your username:");
            String userName = reader.nextLine();
            System.out.print("Your first name:");
            String firstName = reader.nextLine();
            System.out.print("Your last name:");
            String lastName = reader.nextLine();
            System.out.print("Your password:");
            String password = reader.nextLine();
            createUser(userName, firstName, lastName, password);
        }
        login.logIn();
        this.user = login.returnUser();
        this.user.getUsersToDoList().printToDoList();

        while (true) {
            System.out.println("");
            System.out.println("1: Add a to-do item.");
            System.out.println("2. Remove a to-do item.");
            System.out.println("3. Print a list of my to-do items.");
            System.out.println("4. Quit and save");
            System.out.print("Type the number of desired action: ");

            String input = reader.nextLine();

            if (input.equals("4")) {
                saveUsersList();
                System.out.println("Quitting!");
                break;
            } else if (input.equals("1")) {
                System.out.println("What would you like to add?");
                String add = reader.nextLine();
                toDo item = new toDo(add);
                this.user.getUsersToDoList().addToDo(item);
            } else if (input.equals("2")) {
                if (this.user.getUsersToDoList().getList().isEmpty()) {
                    System.out.println("List is empty.");
                    continue;
                }
                System.out.println("");
                this.user.getUsersToDoList().printToDoList();
                System.out.print("Type the index of the item you wish to remove: ");
                int remove = Integer.parseInt(reader.nextLine());
                this.user.getUsersToDoList().removeToDo(remove);
            } else if (input.equals("3")) {
                System.out.println("");
                this.user.getUsersToDoList().printToDoList();
            }
        }
    }

    public void createUser(String userName, String firstName, String lastName, String password) {
        storage.createUser(userName, firstName, lastName, password);
    }

    public void saveUsersList() {
        storage.getToDoLists().put(login.returnUsername(), this.user.getUsersToDoList());
        storage.saveUsersToDoLists();
    }
}

public class Login implements LoginInterface {

    private User user;
    private final Storage storage;
    private Scanner reader;
    private String username;

    public Login(StorageInstance si) {
        this.storage = si.getStorage();
        this.reader = new Scanner(System.in);
    }

    public void logIn() {
        System.out.println("Username:");
        this.username = reader.nextLine();
        System.out.println("Password:");
        String password = reader.nextLine();
        try {
            validateUser(this.username, password);
                System.out.println("Welcome " + user.getFirstName() + "!");
        } catch (NullPointerException npe) {
            System.out.println("Incorrect username or password. Please try again!");
            this.logIn();
        }
    }

    public User returnUser() {
        return this.user;
    }

    public String returnUsername() {
        return this.username;
    }

    public void validateUser(String username, String password) {
        storage.validateUsername(username, password);
        this.user = storage.returnUser();
    }
}

public class StorageInstance {
    private Storage storage;


    public StorageInstance() {
        this.storage = new Storage();
    }

    public Storage getStorage() {
        return this.storage;
    }
}

public interface InterfaceUI {

    public void createUser(String userName, String firstName, String lastName, String password);

    public void saveUsersList();
}

public interface LoginInterface {

    public void validateUser(String username, String password);
}

Answer 1

You improved the program by a lot but then misunderstood some of @TorbenPutkonen's suggestions. As it turns out, the program runs just as fine when you change all StorageInstances to Storage and remove the interfaces you added.

The Storage instance

Let's get the StorageInstance out of the way first. Consider a line like this

Storage store = new Storage();

What happens is that you use the constructor of the Storage class to create a new object, and that object is instance of that class: store is an instance of Storage.
The procedure would then be to create a Storage object at program start and pass that to the constructors of Login and UI, like so:

 // in main()
Storage s = new Storage();
new UI(s).start();

// in the UI class
private final Storage storage;
private Login login;

public UI(Storage s) {
    this.storage = s;
    this.login = new Login(s);
}

// in the Login class
private final Storage storage;

public Login(Storage s) {
    this.storage = s;
}

This also ensures that every party involved gets the same Storage object/instance. Read this question and it should be clear why.

The interfaces

Using an interface for one class only isn't any different from adding the methods that would be in that interface as public methods to that class. They're only useful if added to multiple different classes. This way the implementing class tells the world: "I promise that I can do everything that this interface defines". For a common example, look up the Comparable interface.

Honestly, I'm not sure if you really need to do anything with interfaces or DI here. If you don't plan on adding more features to the whole program, I'd say leave it be because YAGNI. Keep it short and simple.

Other things to note

• No need to use getters/setters inside the class that implements them. You can directly access the field inside that class.
• The load methods in storage save an empty HashMap to the file and then read that file if that file didn't exist i.e. on the first start. Instead, you could just create an empty HashMap as you do and return right away.
• Classes always start with a capital letter. toDo doesn't.
• Normal class members always start with a non-capital letter. UsersToDoLists and UsernamesAndPasswords don't.
• Storage.map should be renamed to something like Storage.allUsers.
• fisTwo, oosTwo, fosTwo, oisTwo are suffixed with "Two" for no reason. If there was a reason they should be given better names, but here you can just remove the "-Two" from the name
• Storage.saveUserNamesAndPasswords() has an unused argument.
• Integer.parseInt() throws an exception when no number is inputted. This should be caught (main UI, option 2).
• Login.logIn() uses recursion. A malicious user might spam the interface with garbage, causing the program to crash because it runs out of stack space. It's better to use an infinite loop instead, then you can just break out or return on success
• User.passwordEquals() returns a Boolean object when it could also return the boolean primitive. Doesn't make a difference here but there's also no gain in using the object version.

One last note on the design: It seems odd to me that the Storage also keeps track of the currently logged in user. Make a copy or a branch if you use git and try moving that functionality into the UI or somewhere else, so that the Storage's only purpose is to load and save your database.

EDIT: The Hash map

In loadUserNamesAndPasswords (as example, same in loadUserToDoLists) you do this:

    public void loadUserNamesAndPasswords(File file) {
        if (file.length() == 0) {
            map = new HashMap<>();
            this.saveUserNamesAndPasswords(map);
        }
        try {
            FileInputStream fis = new FileInputStream(UsernamesAndPasswords);
            // ...
        } catch (Exception e) {
            // ...
        }
    }

What you're doing is checking if the database file exists and if it doesn't, you create an empty HashMap "map" and save that. The file is always empty in that case, so why bother creating the InputStreams and reading the empty file? Just put a return at the end of the if block. Something that's up for choice IMO is not even saving at that point, i.e. removing the call to saveUserNamesAndPasswords(). There's no data to be saved and you'll save to the file on program exit anyways. The rest of the program only cares about map being non-null. In the end, the method looks something like this:

    public void loadUserNamesAndPasswords(File file) {
        if (file.length() == 0) {
            map = new HashMap<>();
            this.saveUserNamesAndPasswords(map); // optional
            return; // this is what I meant
        }
        try {
            FileInputStream fis = new FileInputStream(UsernamesAndPasswords);
            // ...
        } catch (Exception e) {
            // ...
        }
    }

Answer 2

Prefer interfaces to implementations

    private HashMap<String, ToDoList> toDoLists;
    private HashMap<String, User> map;

You should almost never use an implementation as the type. So these should be

    private Map<String, ToDoList> toDoLists;
    private Map<String, User> map;

Then if you change the implementation, you only have to change at initialization. Also, this way, you don't accidentally use a HashMap specific method where one of the Map methods would do.

    public void saveUserNamesAndPasswords(HashMap<String, User> loginInfo) {

Same feedback.

    public void saveUserNamesAndPasswords(Map<String, User> loginInfo) {

Even more important here, as there's no need to limit this to only accept HashMap. It will work with any Map if declared this way.

It is good to get in the habit of using the interface as the type. There may be times to break that habit, but I see no reason to do so here.

try-with-resources

        try {
            FileOutputStream fosTwo = new FileOutputStream(UsersToDoLists);
            ObjectOutputStream oosTwo = new ObjectOutputStream(fosTwo);

            oosTwo.writeObject(this.toDoLists);
            oosTwo.flush();
            oosTwo.close();
            fosTwo.close();

This could be better written

        try (
            FileOutputStream fosTwo = new FileOutputStream(UsersToDoLists);
            ObjectOutputStream oosTwo = new ObjectOutputStream(fosTwo)) {

            oosTwo.writeObject(this.toDoLists);
            oosTwo.flush();

You should not manually manage resources such that an exception will prevent the resource from being closed. You could move the close lines into a finally block, but the try-with-resources form will handle that for you.