-1
\$\begingroup\$

I've written simple ToDo Console Application with option to create account and log into.

Actually, this option is unnecessary, because you have to create an account, but I don't know how to have the code interact with a DataBase yet. I wanted to be as much OOP as it can be. Thanks for all suggestions.

Main class

public class Main {
public static void main(String[] args) {
    ToDoEngine engine = new ToDoEngine();
    engine.displayMainMenu();
    engine.displayUserMenu();
    }
}

AccountMaker

import java.util.HashMap;
import java.util.Scanner;

public class AccountMaker {

private HashMap<String, User> loginDetails = new HashMap<>();
private String login, password;
private Scanner input = new Scanner(System.in);


void inputLoginAndPassword() {
    System.out.println("Input your login");
    login = input.next();

    System.out.println("Input your password");
    password = input.next();
}

void createAccount() throws InterruptedException {
    User newUser = new User(login, password);
    loginDetails.put(login, newUser);
    System.out.println("Account has been created successfully");
    Thread.sleep(2000);
}

HashMap<String, User> getLoginDetails() {
    return loginDetails;
}

}

AccountLogger

import java.util.Scanner;

public class AccountLogger {

private AccountMaker accountMaker;
private Scanner input = new Scanner(System.in);
private String login, password;


AccountLogger(AccountMaker accountMaker) {
    this.accountMaker = accountMaker;
}


void inputLoginAndPassword() {
    System.out.println("Input your login");
    login = input.next();

    System.out.println("Input your password");
    password = input.next();
}

boolean isLoginDataIncorrect() {
    User user = accountMaker.getLoginDetails().get(login);
    try {
        if (user.getPassword().equals(password)) {
            System.out.println("You've logged in.");
            return false;
        } else {
            System.out.println("Bad login or password");
        }
    } catch (NullPointerException e) {
        System.out.println("Bad login or password");
    }
    return true;
}
}

User

public class User {
private Tasker tasker;
private String login;
private String password;

User(String login, String password) {
    this.login = login;
    this.password = password;
}

User(Tasker tasks) {
    this.tasker = tasks;
}

void addNewTask(String task) {
    tasker.createTask(task);
}

void deleteTask(String task) {
    tasker.deleteTask(task);
}

void showAllTasks() {
    tasker.showAllTasks();
}

String getPassword() {
    return password;
}

}

Tasker

import java.util.ArrayList;
import java.util.List;

public class Tasker {

private List<String> listOfTasks = new ArrayList<>();


void createTask(String task) {
    listOfTasks.add(task);
}

void deleteTask(String task) {
    listOfTasks.remove(task);
}

void showAllTasks() {
    for (String listOfTask : listOfTasks) {
        System.out.println(listOfTask);
    }
}
}

ToDoEngine

import java.util.InputMismatchException;
import java.util.Scanner;

public class ToDoEngine {

private Scanner input = new Scanner(System.in);
private AccountMaker accountMaker = new AccountMaker();
private AccountLogger accountLogger = new AccountLogger(accountMaker);
private boolean loopIsTrue = true;
private Tasker tasks = new Tasker();
private User user = new User(tasks);

void displayMainMenu() {
    System.out.println("What do you wanna to do?");
    System.out.println("1. Add Account 2. Log into my account");

    while (loopIsTrue) {
        try {
            getOptionsOfMainMenu(input.nextInt());
        } catch (InputMismatchException e) {
            System.out.println("You've inputed something wrong!");
            System.out.println("What do you wanna to do?");
            System.out.println("1. Add Account 2. Log into my account");
            input.next();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

private void getOptionsOfMainMenu(int option) throws InterruptedException {
    if (option >= 1 && option <= 2) {
        switch (option) {
            case 1:
                accountMaker.inputLoginAndPassword();
                accountMaker.createAccount();
                System.out.println("Now let's log into you account.");
            case 2:
                while (loopIsTrue) {
                    accountLogger.inputLoginAndPassword();
                    if (!accountLogger.isLoginDataIncorrect()) {
                        loopIsTrue = false;
                    }
                }
                break;
        }
    }
}

public void displayUserMenu() {
    loopIsTrue = true;
    while (loopIsTrue) {
        System.out.println("What do you wanna to do?");
        System.out.println("1. Add task 2. Show my tasks 3. Delete task 4. Exit");
        getOptionsOfUserMenu(input.nextInt());
    }
}

private void getOptionsOfUserMenu(int option) {
    if (option >= 1 && option <= 4) {
        switch (option) {
            case 1:
                System.out.println("Write down your task.");
                input.nextLine();
                user.addNewTask(input.nextLine());
                System.out.println("Task was added.");
                break;
            case 2:
                System.out.println("----------------");
                System.out.println("YOUR TASK LIST:");
                user.showAllTasks();
                System.out.println("----------------");
                break;
            case 3:
                System.out.println("Write down your task that you want to delete from task list.");
                input.nextLine();
                user.deleteTask(input.nextLine());
                System.out.println("Task was deleted");
                loopIsTrue = false;
                break;
            case 4:
                loopIsTrue = false;
                break;
        }
    }
}
}

Here is github link to repository that contains these classes.

\$\endgroup\$
0
\$\begingroup\$

There are IMO some issues in your code so I'll point out the things you may want to change :

  1. You are mixing the inner workings of your application with its view (how it's presented), you should avoid this most of the time (unless you are going for a simple script). By splitting the view from the logic, you can change the way data are shown to the user without touching any line of codes that take care of the behaviour.
  2. The separation of your classes feels a bit random... for instance, why do you have an AccountMaker and an AccountLogger ? what was the rationale behind splitting those two ? what does the Tasker class bring you ?
  3. You are always using the default visibility for your methods, make your methods public
  4. Your code always talk about Account but then, the model is stored as User
  5. While you abstract some things, you are not using any abstraction for your tasks... they are simply stored as String, adding behaviour to your task (for example, a due date) would have been easier with Tasks objects.
  6. You should use dependency injection to make your code OO. By using dependency injection, you'll reduce the coupling between your objects making them easier to test and extend. You can take a look at the following SO answer for a brief explanation : https://stackoverflow.com/a/6085922/7454623
  7. Never store password as a clear text, you should hash it... if you don't know how to do it, there are plenty of library that can do it, in the standard lib you can take a look at the following class : https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html
  8. Methods' name should make their intent clear, for example, methods that start with a get, compute, create, build should return a value... as such, the method getOptionsOfMainMenu shouldn't be void but rather return a list of options :)
  9. InterruptedException isn't a meaningless exception, so you shouldn't let it being thrown around and then use printStackTrace on it at a completely random place. Those exceptions means your program have receive a message from a thread telling it to stop while it was waiting.
  10. ...well, actually, why are you even calling Thread.sleep there ? ^^'
  11. Why are you creating a Tasker in ToDoEngine ?? Aren't tasks supposed to be linked to the user ?
  12. Avoid the default package, it's considered bad practice (and actually gives a warning in most IDE) : https://stackoverflow.com/questions/7849421/is-the-use-of-javas-default-package-a-bad-practice
  13. Don't declare two variables on the same line, it's harder to read ;)

While I could try to improve your existing code, I'll rather tell you from step one how I'd do it myself.

Let's start from the basics : You want to make a 'ToDo' application with multiple users (stored in a database) that each have a list of tasks.
This application will be printed to the console (at least at the beginning).

At this point, you should make a quick UML class diagram :
We'll have a UserRepository which will fetch the users from our database (for now, we'll go with a Map), as we have user, we'll manipulate User objects and they'll themselves store multiple Tasks. Plus, we'll have a ToDoEngine that'll mix every previous objects together and will be responsible of managing the various inputs. Finally, there'll be a ToDoView that'll print the data given by the ToDoEngine and send the user input to the ToDoEngine.

Here is a quick example of some of the classes :

class User {
    private String name;
    private String password;
    private List<Task> tasks;

    public User(final String name, final String password) {
        this(name, password, new ArrayList<Task>());
    }

    public User(final String name, final String password, final List<Task> tasks) {
        if (name == null || name.isEmpty()) {
            throw new IllegalArgumentException("name must not be empty");
        }
        this.name = name;
        this.password = password;
        this.tasks = Objects.requireNonNull(tasks, "tasks must not be null");
    }

    public String getName() {
        return name;
    }

    public String getPassword() {
        return password;
    }

    public void addNewTask(final Task task) {
        tasks.add(task);
    }

    public boolean deleteTask(final Task task) {
        return tasks.remove(task);
    }

    public List<Task> getTasks() {
        return Collections.unmodifiableList(tasks);
    }
}

public interface UserRepository {
    List<User> findAll();

    User findByUserNameAndPassword(final String name, final String password);

    User save(final User user);
}

public class MapBasedUserRepository implements UserRepository {
    private final Map<String,  User> loginDetails = new HashMap<>();

    @Override
    public List<User> findAll() {
        return new ArrayList<>(loginDetails.values());
    }

    @Override
    public User findByUserNameAndPassword(final String name, final String password) {
        User user = loginDetails.get(name);
        if (user != null && user.getPassword().equals(password)) {
            return user;
        }
        return null;
    }

    @Override
    public User save(final User user) {
        Objects.requireNonNull(user, "user must not be null");
        loginDetails.put(user.getName(), user);
        return user;
    }
}

public class ToDoEngine {

    private final UserRepository userRepository;

    private User connectedUser;

    public ToDoEngine(final UserRepository userRepository) {
        this.userRepository = Objects.requireNonNull(userRepository, "userRepository must not be null");
    }

    private String hashPassword(final String initialPassword) {
        // TODO
    }

    public User login(final String name, final String password) {
        User res = userRepository.findByUserNameAndPassword(name, hashPassword(password));
        if (res == null) {
            throw new IllegalStateException();
        }
        connectedUser = res;
        return res;
    }

    public boolean logout() {
        // TODO
    }

    public List<String> getMainMenuOptions() {
        // TODO 
    }

    public List<String> getOptionsOfUserMenu() {
        // TODO
    }

    public void addNewTask(final String taskName) {
        connectedUser.addNewTask(new Task(taskName));
    }

    public void deleteTask(final String taskName) {
        // TODO
    }

    public List<Task> getAllTasksFromUser() {
        return connectedUser.getTasks();
    }
}

Hope it helps :) On a last note, you should try to write some unit tests ;) it really helps getting better at coding ^^

\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for the answer. I'm going to fix the program by taking into consideration your points! :) \$\endgroup\$ – must Feb 10 '18 at 22:24
  • \$\begingroup\$ @must no problem :) forgot to add a point 14 : try to validate the fields in the constructor, as I've done in the sample code. If you have not sure of the resulting code, don't hesitate to ask a follow-up question ;) \$\endgroup\$ – Ronan Dhellemmes Feb 11 '18 at 9:18
  • \$\begingroup\$ Well, I don't know exactly what you are saying at 5 point. Should I delete Tasker or what? \$\endgroup\$ – must Feb 11 '18 at 14:51
  • \$\begingroup\$ In point 6, you are saying about dependency injection. Actually, I used it here: private Account account = new Account(new Tasker()); // I don't know where else I should use it. \$\endgroup\$ – must Feb 11 '18 at 14:53
  • \$\begingroup\$ @must you should use dependency injection in your ToDoEngine... and yeah IMO Tasker is useless... your User can simply have a field that is a List (or Collection) of tasks... as shown in the code sample I made ;) \$\endgroup\$ – Ronan Dhellemmes Feb 11 '18 at 21:28

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.