2
\$\begingroup\$

This is the first project I have ever undertaken and now that I've gotten it to a point I feel comfortable with, I would love to get some review and tips for any improvement on it! I am especially interested in getting tips on the design, readability and unit testing part of my code and any and all tips for improvement are welcome!

I have purposefully written all the data to files in the Storage-class, but my next step is going to be to implement an SQL database along with a GUI of some sort if I happen to get my project reviewed!

I have not yet written any unit tests for the Storage-class and I am wondering whether it is usually even done? I suppose it is since they must return correct information, but I wanted to ask anyway...

If there is anything that is unclear in the code, please do ask and I will clarify!

Here is a short summary of each of the classes function.

Login-class: Is responsible for the logging in of a user and returning the correct user along with a correct ToDoList.

RegisterNewUsernameAndPassword-class: Quite obvious, but it is responsible for creating a new user.

Storage-class: Responsible for writing the Users and ToDoLists to a file so they can be saved and loaded. Also forwards the information to the Login-class and some other classes.

ToDoList-class: Responsible for the creation of a ToDoList for the user and the associated methods.

ToDoListTest-class: Unit tests for the ToDoList class.

User-class: Defines the parameters for a new User and contains setters and getters.

toDo-class: Every item added to a ToDoList is a toDo-item and this class is responsible for the creation of those.

UI-class: The user interface which transmits the information to the user via a console presently.

public class UI {

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

    public UI() {
        this.reader = new Scanner(System.in);
        this.storage = new Storage();
        this.login = new Login();
        this.registerNew = new RegisterNewUsernameAndPassword();
    }

    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();
            registerNew.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")) {
                storage.getToDoLists().put(login.returnUsername(), this.user.getUsersToDoList());
                storage.saveUsersToDoLists(storage.getToDoLists());
                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 class Login {

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

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

    public void logIn() {
        storage.loadUserNamesAndPasswords(storage.getUsernamesAndPasswordsFile());
        storage.loadUsersToDoLists(storage.getUsersToDoListsFile());
        System.out.println("Username:");
        this.username = reader.nextLine();
        System.out.println("Password:");
        String password = reader.nextLine();
        try {
            if (storage.getUserNamesAndPasswords().get(username).passwordEquals(password) != null) {
                this.user = storage.getUserNamesAndPasswords().get(username);
                this.user.setList(storage.getToDoLists().get(username));
                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 class User implements Serializable {

    private String firstName;
    private String lastName;
    private String password;
    private ToDoList toDoList;

    public User(String firstName, String lastName, String password) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.password = password;
        this.toDoList = new ToDoList();
    }

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public ToDoList getUsersToDoList() {
        return this.toDoList;
    }

    public void setList(ToDoList list) {
        this.toDoList = list;
    }

    public Boolean passwordEquals(String password) {
        return this.password.equals(password);
    }
} 

public class Storage {

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

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

    public void saveUsersToDoLists(HashMap<String, ToDoList> usersToDoLists) {
        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(toDoLists);
        }
        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 class RegisterNewUsernameAndPassword {
    
    private Storage storage;
    private User user;


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

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

public class ToDoListTest {
    
    @Test
    public void addToDo(){
        ToDoList todolist = new ToDoList();
        toDo todo = new toDo("Test");
        todolist.addToDo(todo);
        assertTrue(todolist.getList().contains(todo));
    }
    @Test
    public void removeToDo() {
        ToDoList todolist = new ToDoList();
        toDo todo = new toDo("Test");
        todolist.addToDo(todo);
        todolist.removeToDo(1);
        assertFalse(todolist.getList().contains(todo));
    }
    @Test
    public void listIsEmptyWhenCreated() {
        ToDoList todolist = new ToDoList();
        assertTrue(todolist.getList().isEmpty());
    }

    @Test
    public void getList() {
        ToDoList todolist = new ToDoList();
        ArrayList<toDo> list = new ArrayList<>();
        assertEquals(list, todolist.getList());
    }

    @Test
    public void printToDoList() {
        ToDoList todolist = new ToDoList();
        toDo todo = new toDo("Test");
        todolist.addToDo(todo);
        PrintStream oldOut = System.out;
        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        System.setOut(new PrintStream(baos));
        todolist.printToDoList();
        System.setOut(oldOut);
        String output = baos.toString();
        assertTrue(output.contains("1: Test"));
    }

    @Test
    public void printEmptyToDoList() {
        ToDoList todolist = new ToDoList();
        PrintStream oldOut = System.out;
        ByteArrayOutputStream baos = new ByteArrayOutputStream();
        System.setOut(new PrintStream(baos));
        todolist.printToDoList();
        System.setOut(oldOut);
        String output = baos.toString();
        assertTrue(output.contains("List is empty."));
    }
}

public class toDo implements Serializable {

    private String name;

    public toDo(String name) {
        this.name = name;
    }

    public void setName(String setName) {
        this.name = setName;
    }

    public String getName() {
        return this.name;
    }

    public String toString() {
        return this.name;
    }
}

public class ToDoList implements Serializable {

    private ArrayList<toDo> toDoList;

    public ToDoList() {
        this.toDoList = new ArrayList<>();
    }

    public void addToDo(toDo toDo) {
        this.toDoList.add(toDo);
    }

    public void removeToDo(int toDo) {
        try {
            this.toDoList.remove(toDo - 1);
        } catch (IndexOutOfBoundsException e) {
            System.out.println("The index you have entered is invalid.");
            System.out.println("Please enter a number between or equal to 1 or " + toDoList.size() + ".");
        }
    }

    public void printToDoList() {
        if (toDoList.isEmpty()) {
            System.out.println("List is empty.");
        } else {
            int i = 1;
            for (toDo todo : toDoList) {
                System.out.println(i + ": " + todo);
                i++;
            }
        }
    }
    public ArrayList<toDo> getList() {
        return this.toDoList;
    }
}
\$\endgroup\$
1
3
\$\begingroup\$

Welcome to code review! There is a lot of code and a lot to say, but I will concentrate on Storage as it is a fairly central component that affects the quality of all other classes.

You should study the SOLID principles, especially the D (dependency inversion). You should treat Storage as a service that is provided to each other class. Not something that the classes have to create themselves. So create a class that bootstraps one Storage instance and passes that instance to the UI, Login and whoever needs it.

When you think about Storage as a service, you need to identify the operations that are needed by its clients. Knowing the location of the usernames and passwords file hardly is something that is relevant to the user interface or login, right? Thus you shouldn't expose the getUsernamesAndPasswordsFile(). Just let Storage handle all the file related stuff privately. When you have identified the required storage operations, define them in an interface and make the classes use the interface, not the concrete class that implements it.

RegisterNewUsernameAndPassword should not be a class. Its name describes an action, therefore it should be a method in Storage. You should also consider whether it makes sense to complicate your application with user accounts when it is not really usable in shared environment. It's a beginner project so you maybe should concentrate first on being able to create a good single user application before submitting yourself to the multitude of complications that arise when many users access a single application.

\$\endgroup\$
2
  • \$\begingroup\$ So I should make a “StorageBootstrapper”-class, and then create an instance of Storage in that class and thus create an instance of “StorageBootstrapper” in each class that needs the services of the storage? I’m not sure what you mean by passing other than creating an instance of that class inside another? Do you mean that I would create an interface with the methods that I need and then use an instance of the StorageBootstrapper-class inside those methods and via that use methods that are defined in the storage-class itself thus hiding the details of, the file-related stuff for example? \$\endgroup\$
    – Apelli
    Aug 14 at 17:07
  • \$\begingroup\$ I mean that UI (and other classes that need Storage) should have a constructor that takes a Storage instance as a parameter. \$\endgroup\$ Aug 16 at 4:22

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.