ToDoApp - adding users, managing tasks, getting tasks

Question

I've implemented a console ToDo application. I would like to hear some opinions about it, what should I change etc.

About project:

It is intended to manage everyday tasks. Application contains:

• adding users to database
• adding tasks to database
• getting list of user's tasks

It's my second project in which I use mockito, lombok, junit, slf4j. I tried to use MVC pattern. I've written unit tests for my controller, too.

Let's start from package MODEL (there is nothing interesting, but I want to add every class). User class (model)

package model;
import lombok.*;
@RequiredArgsConstructor
@Getter
@Setter
@EqualsAndHashCode
public class User {
 private final String name;
 private final String password;
 private int ID;
}

Task class (model)

package model;
import lombok.*;
@Getter
@Setter
@AllArgsConstructor
@ToString
@NoArgsConstructor
@EqualsAndHashCode
public class Task {
 private String taskName;
}

Here is my VIEW package:

ToDoView class(view)

package view;
import controller.ToDoEngine;
import model.Task;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.sql.SQLException;
import java.util.InputMismatchException;
import java.util.List;
import java.util.Scanner;
public class ToDoView {
 private static final int LOGIN = 1;
 private static final int REGISTER = 2;
 private Logger logger;
 private ToDoEngine toDoEngine;
 private Scanner input;
 private int option;
 public ToDoView(ToDoEngine toDoEngine) {
 this.toDoEngine = toDoEngine;
 logger = LoggerFactory.getLogger(ToDoView.class);
 input = new Scanner(System.in);
 }
 public void executeMainMenu() {
 logger.info("--MAIN MENU--");
 logger.info("1. Sign in");
 logger.info("2. Sign up");
 boolean isInvalid = true;
 while (isInvalid) {
 try {
 option = input.nextInt();
 if (option == 1 || option == 2) {
 isInvalid = false;
 switch (option) {
 case LOGIN:
 executeLoginCase();
 break;
 case REGISTER:
 executeRegistrationCase();
 break;
 }
 }
 } catch (InputMismatchException e) {
 logger.error("Try again");
 input.next();
 } catch (SQLException e) {
 e.printStackTrace();
 }
 }
 }
 private void executeLoginCase() throws SQLException {
 String username, password;
 boolean isInvalid = true;
 do {
 logger.info("Put your username and password");
 logger.info("User: ");
 username = input.next();
 logger.info("Password: ");
 password = input.next();
 if (toDoEngine.signIn(username, password)) {
 logger.info("You've logged in");
 executeUserCase();
 isInvalid = false;
 } else
 logger.info("Bad login or password, try again");
 }
 while (isInvalid);
 }
 private void executeRegistrationCase() throws SQLException {
 String username, password;
 logger.info("Put your username and password");
 logger.info("User: ");
 username = input.next();
 logger.info("Password: ");
 password = input.next();
 if (toDoEngine.createUser(username, password))
 logger.info("User created successfully, now you can sign in!");
 }
 private void executeUserCase() throws SQLException {
 do {
 logger.info("1. Add task");
 logger.info("2. Remove task");
 logger.info("3. Get all tasks");
 logger.info("4. Quit");
 option = input.nextInt();
 executeUserOptions(option);
 }
 while (option != 4);
 }
 private void executeUserOptions(int option) throws SQLException {
 switch (option) {
 case 1:
 logger.info("Input task");
 String taskName = input.next();
 toDoEngine.addTask(taskName);
 break;
 case 2:
 logger.info("Input task");
 taskName = input.next();
 toDoEngine.deleteTask(taskName);
 break;
 case 3:
 List<Task> tasks = toDoEngine.getTasks();
 for (Task task : tasks)
 logger.info(String.valueOf(task));
 break;
 }
 }
}

Controller package

ToDoEngine class (controller)

package controller;
import model.Task;
import model.User;
import repository.TaskActions;
import repository.UserActions;
import java.sql.SQLException;
import java.util.List;
public class ToDoEngine {
 private TaskActions taskActions;
 private UserActions userActions;
 private User connectedUser;
 public ToDoEngine(UserActions userStorage, TaskActions taskStorage) {
 this.taskActions = taskStorage;
 this.userActions = userStorage;
 }
 public boolean signIn(String username, String password) throws SQLException {
 connectedUser = new User(username, password);
 if (!userActions.signIn(connectedUser)) {
 return false;
 }
 connectedUser.setID(retrieveConnectedUserID(connectedUser));
 return true;
 }
 private int retrieveConnectedUserID(User connectedUser) throws SQLException {
 return userActions.retrieveUserID(connectedUser);
 }
 public boolean createUser(String username, String password) throws SQLException {
 return userActions.createUser(new User(username, password));
 }
 public void addTask(String taskName) throws SQLException {
 taskActions.addTask(new Task(taskName), connectedUser);
 }
 public void deleteTask(String taskName) throws SQLException {
 taskActions.deleteTask(new Task(taskName), connectedUser);
 }
 public List<Task> getTasks() throws SQLException {
 return taskActions.getTasks(connectedUser);
 }
}

Repository package. I created 2 interfaces - UserActions and TaskActions. That's beacuse I wanted to have everything organized. (I'm not gonna paste interfaces. Just classes that implement them)

TaskRepository class (repository)

package repository;
import model.Task;
import model.User;
import java.sql.*;
import java.util.ArrayList;
import java.util.List;
import java.util.TimeZone;
public class TaskRepository implements TaskActions {
 private Connection connection;
 public TaskRepository() throws SQLException {
 connection = DriverManager.getConnection("jdbc:mysql://localhost:3306/todoapp?autoReconnect=true&serverTimezone=" + TimeZone.getDefault().getID(), "root", "password");
 }
 public boolean addTask(Task task, User user) throws SQLException {
 if (task == null)
 return false;
 PreparedStatement preparedStatement = connection.prepareStatement("INSERT into tasks (task,idUser) values (?,?) ");
 preparedStatement.setString(1, task.getTaskName());
 preparedStatement.setInt(2, user.getID());
 preparedStatement.executeUpdate();
 return true;
 }
 public boolean deleteTask(Task task, User user) throws SQLException {
 if (!doesTaskExists(task))
 return false;
 PreparedStatement preparedStatement = connection.prepareStatement("DELETE from tasks WHERE idUser=? AND task=?");
 preparedStatement.setInt(1, user.getID());
 preparedStatement.setString(2, task.getTaskName());
 preparedStatement.executeUpdate();
 return true;
 }
 public List<Task> getTasks(User connectedUser) throws SQLException {
 PreparedStatement preparedStatement = connection.prepareStatement("SELECT * FROM tasks where idUser=?");
 preparedStatement.setInt(1, connectedUser.getID());
 ResultSet result = preparedStatement.executeQuery();
 List<Task> tasks = new ArrayList<Task>();
 while (result.next()) {
 Task task = new Task();
 task.setTaskName(result.getString("task"));
 tasks.add(task);
 }
 return tasks;
 }
 public boolean doesTaskExists(Task task) throws SQLException {
 PreparedStatement preparedStatement = connection.prepareStatement("SELECT task FROM tasks WHERE task=?");
 preparedStatement.setString(1, task.getTaskName());
 ResultSet result = preparedStatement.executeQuery();
 return result.next();
 }
}

UserRepository (repository)

package repository;
import model.User;
import java.sql.*;
import java.util.TimeZone;
public class UserRepository implements UserActions {
 private Connection connection;
 public UserRepository() throws SQLException {
 connection = DriverManager.getConnection("jdbc:mysql://localhost:3306/todoapp?autoReconnect=true&serverTimezone=" + TimeZone.getDefault().getID(), "root", "password");
 }
 public boolean createUser(User user) throws SQLException {
 if (doesUserWithThatUsernameExist(user))
 return false;
 PreparedStatement preparedStatement = connection.prepareStatement("INSERT into user (username,password) values (?,?)");
 preparedStatement.setString(1, user.getName());
 preparedStatement.setString(2, user.getPassword());
 preparedStatement.executeUpdate();
 return true;
 }
 public boolean signIn(User user) throws SQLException {
 PreparedStatement preparedStatement = connection.prepareStatement("SELECT COUNT(username) FROM user WHERE username=? AND password=? LIMIT 0,1");
 preparedStatement.setString(1, user.getName());
 preparedStatement.setString(2, user.getPassword());
 ResultSet result = preparedStatement.executeQuery();
 int count = 0;
 if (result.next())
 count = result.getInt(1);
 return count >= 1;
 }
 public int retrieveUserID(User user) throws SQLException {
 PreparedStatement preparedStatement = connection.prepareStatement("SELECT id FROM user WHERE username=? AND password=?");
 preparedStatement.setString(1, user.getName());
 preparedStatement.setString(2, user.getPassword());
 ResultSet result = preparedStatement.executeQuery();
 if (result.next())
 user.setID(result.getInt("id"));
 return user.getID();
 }
 private boolean doesUserWithThatUsernameExist(User user) throws SQLException {
 PreparedStatement preparedStatement = connection.prepareStatement("SELECT username FROM user WHERE username=?");
 preparedStatement.setString(1, user.getName().toUpperCase());
 ResultSet result = preparedStatement.executeQuery();
 return result.next();
 }
}

Main class:

import controller.ToDoEngine;
import repository.TaskRepository;
import repository.UserRepository;
import view.ToDoView;
import java.sql.SQLException;
public class Main {
 public static void main(String[] args) throws SQLException {
 UserRepository userRepository = new UserRepository();
 TaskRepository taskRepository = new TaskRepository();
 ToDoEngine toDoEngine = new ToDoEngine(userRepository,taskRepository);
 ToDoView toDoView = new ToDoView(toDoEngine);
 toDoView.executeMainMenu();
 }
}

Here are my unit tests:

package controller;
import model.Task;
import model.User;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import repository.TaskActions;
import repository.UserActions;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
public class ToDoEngineTest {
 @Mock
 TaskActions taskActionsMock;
 @Mock
 UserActions userActionsMock;
 private ToDoEngine toDoEngine;
 @Before
 public void setup() {
 MockitoAnnotations.initMocks(this);
 toDoEngine = new ToDoEngine(userActionsMock, taskActionsMock);
 }
 @Test
 public void createUser() throws SQLException {
 User user = new User("admin", "123");
 toDoEngine.createUser("admin", "123");
 verify(userActionsMock).createUser(user);
 }
 @Test
 public void getTasks() throws SQLException {
 List<Task> tasks = new ArrayList<>();
 User user = new User("admin", "123");
 Task task = new Task("wash");
 tasks.add(task);
 when(taskActionsMock.getTasks((User) any())).thenReturn(tasks);
 List<Task> actual = toDoEngine.getTasks();
 List<Task> expected = taskActionsMock.getTasks(user);
 assertEquals(expected, actual);
 }
 @Test
 public void signIn() {
 // TODO: 2018年09月06日
 }
 @Test
 public void addTask() throws SQLException {
 toDoEngine.signIn("admin", "123");
 Task taskName = new Task("wash");
 User user = new User("admin", "123");
 verify(userActionsMock).signIn(user);
 toDoEngine.addTask("wash");
 verify(taskActionsMock).addTask(taskName, user);
 }
 @Test
 public void deleteTask() throws SQLException {
 toDoEngine.signIn("admin", "123");
 Task taskName = new Task("wash");
 User user = new User("admin", "123");
 verify(userActionsMock).signIn(user);
 toDoEngine.deleteTask("wash");
 verify(taskActionsMock).deleteTask(taskName, user);
 }
}

• Mast: Welcome to Code Review! Does it currently work as intended?
• pipilam: @Mast hey, yes. Everything works fine!
• gervais.b: Hello @pipilam, could you explain your choice of using a Logger instead of System.out or anything else to print messages to the user ?
• pipilam: @gervais.b I've read that using Loggeris more efficient and it is also good practice to use that instead of System.out (thinking about future projects)
• gervais.b: It is indeed a good practice to use a logger to log messages. But logging is for technical purpose (debugging, tracing) but not to display something to your users.

Answer 1

Globally, it works. I have to admit that I am a bit skeptic on your usage of a logger. Because a logger is dedicated to log, some can be asynchronous or use a buffer or are just disabled. Why don't you use the System.out ?

You have some tests and is a good thing. Sadly, you are only testing the ToDoEngine that most of the time delegate to your actions and your actions are mocks. The most complex part of your application (the one with the biggest number of conditions and branches) is the ToDoView. But this one is not tested and this is difficult to do because this class receive inputs and write to the console/logger.

A solution would be to create one abstraction for the "output" and another for the "input". But this still looks strange to me.

---

After looking at a simple description of the MVC pattern, I have the feeling that your view is also your controller:

• Model: Represents problem data and operations to be performed on it.
• View : Presents information from the Model to the user via the display.
• Controller: Interprets inputs from the user and adjust the Model or View accordingly.

-- https://twitter.com/coreload/status/980212512137166848/photo/1

Let's try to move the Scanner into the controller. This one will still change what is displayed to the user via the view:

class ToDoController {
 ToDoView view;
 Scanner in;
 void start() {
 do {
 view.displayMenu();
 action = in.nextInt();
 switch (action) {
 case REGISTER:
 executeRegistrationCase();
 case LOGIN:
 executeLoginCase();
 break;
 default:
 view.showError("Invalid option");
 break;
 }
 } while ( action!=QUIT );
 }
} 

Because the ToDoView is now a dumb component you can live without testing it. The complexity is now in your controller that Interprets inputs from the user and adjust the .. View accordingly.

Another improvement that you can do is to create controllers for each use cases. So that your ToDoController act more as a front controller and drive the whole application without knowing all the details. If we go deeper is this idea, you can end up by having one controller/action for each menu entry and each of them will change a shared model that is the state of your application.

[1] Smart and Dumb components are mainly used in frontend development but the concept is not new.

[2] Front controller is one of the J2EE patterns, "request dispatcher" is also used for the same purpose. They are dedicated to Web applications but can be reused in other kind of systems. http://www.oracle.com/technetwork/java/frontcontroller-135648.html

• pipilam: You mean something like this:? gist.github.com/must1/ab36f8fe3e7dcba078099b059015a379
• pipilam: Hey. I created new controller. I updated gist. Take a look if you can. Thanks.
• gervais.b: Well my idea was more to launch another controller like a subsystem. I'm updating your project with what I have in mind on the little free time I have. Until it is available I dropped the important parts on this gist gist.github.com/gervaisb/b26ebfa8bdbd39e70b7d1e9d8226f90c (And will update the post one completed)
• pipilam: Well, it looks like another project, not mine. That was not what I was about. I wanted to hear what I should change, not how I should write it. I want to work on what I've written. Does the view class looks fine? Just printing messages. What about ToDoController?
• pipilam: I've updated gist. It should be fine right now. Check it out.