MVC - czy dobrze to zrobiłem? ToDoApp - review

Question

Cześć. Zrobiłem ToDoAppkę na podstawie wzorca MVC. Chciałbym abyście sprawdzili, czy ten pull request jest dobry.

Przedtem w klasie ToDoView pobierałem od użytkownika dane. Teraz wiem, ze powinno się to znajdować w kontrolerze, dlatego wszystko to przeniosłem do kontrolera właśnie, a w klasie ToDoView pozostawiłem metody tylko do wypisywania.

Dodałem także TaskController, ponieważ w klasie ToDoController znajdowało się już zbyt wiele rzeczy.

Co o tym myślicie?

Tutaj link do pull requesta: https://github.com/must1/ToDoApplication/pull/4

Answer 1

Edit usunąłem to bo patrzyłem na zły branch i w sumie nie miało to już sensu.

Zerknij na ten przykładzik, zauważ, że widok tylko drukuje komunikaty, a dane pobiera kontroler i zarządza co i kiedy ma być wyświetlane (w zależności od akcji użytkownika):

https://github.com/mbabanes/java-examples/tree/master/src/main/java/console

Comments

W tym sęk, że wprowadziłem zmiany. Wygląd nie pobiera danych tylko kontroler. Jest tak jak napisałeś.

ToDoController toDoController = new ToDoController(userRepository,taskRepository,toDoView);

Nie wiem czy nie popatrzyłeś na starą wersję.

W ToDoView mam 3 metody

package view;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class ToDoView {

    private Logger logger;

    public ToDoView() {
        logger = LoggerFactory.getLogger(ToDoView.class);
    }

    public void executeMainMenu() {
        logger.info("--MAIN MENU--");
        logger.info("1. Sign in");
        logger.info("2. Sign up");
    }


    public void showError(String error) {
        logger.info(error);
    }

    public void showMessage(String message) {
        logger.info(message);
    }
}

///EDIT

Dodałem nową metode do ToDoView

public void showTaskMenu() {
    logger.info("1. Add task");
    logger.info("2. Remove task");
    logger.info("3. Get all tasks");
    logger.info("4. Quit");
}

 

---

Tak zgadza się, patrzyłem na zły branch, przepraszam. 

Wydaje mi się że to:

 toDoView.showMessage("Input task");

jest za bardzo nadużywane. Ten sam tekst powtarzasz kilka razy, a także:

  toDoView.showMessage("Put your username and password");
  toDoView.showMessage("User: ");

 Czy nie lepiej umieścić to po prostu w widoku - na tej zasadzie co w moim przykładzie. A showMessage zostawić tylko do jakiś bardzo krótkich wiadomości w stylu okien dialogowych w gui?

I wydaje mi się, że takie coś:

  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;
    }

Raczej trzeba umieścić w modelu, a w kontrolzerze tylko ewentualnie:

boolean connected = model.sigin(username, password);
if (connected)
   view.showMessage("Connected success!");
else
 view.showError("Wrong values");

Czyli te repozytoria również trzeba by umieścić w modelu.

 

I jeszcze zastanów się czy ten TaskController rzeczywiście jest kontrolerem. Zdaniem kontrolera w mvc jest łączenie użytkownika z widokiem i modelem. A w TaskController masz jakieś zwyczajne przetwarzanie danych.

---

Kompletnie się zagubiłem teraz.

Repozytoria są od bazy danych, więc ciężko je przenieść do modelu.

Przedtem miałem coś takiego zrobione:

connectedUser = new User(username, password);

i to był mój użytkownik z którego korzystałem też tutaj:

private void executeUserOptions(int option) throws SQLException {
    switch (option) {
        case 1:
            toDoView.showMessageAboutInputingTask();
            String taskName = input.next();
            taskController.addTask(taskName, connectedUser);
            break;
        case 2:
            toDoView.showMessageAboutInputingTask();
            taskName = input.next();
            taskController.deleteTask(taskName, connectedUser);
            break;
        case 3:
            List<Task> tasks = taskController.getTasks(connectedUser);
            for (Task task : tasks)
                toDoView.showMessage(String.valueOf(task));
            break;
    }
}

teraz jak stworzyłem nową klasę o nazwie Login i tam przeniosłem tę metodę o której mówiłeś, to wywala mi błąd, ponieważ nie mam connectedUser.

Które w takim razie metody powinienem poprzenosić i jak? Bo się zrobiło takie zamiesznie, że nie wiem jak już to zrobić.

---

Repozytoria są od bazy danych, więc ciężko je przenieść do modelu.

Do modelu w sensie MVC, nie encji bazy danych. Model w MVC odpowiada za logikę, a u Ciebie cześć logiki znajduje się w kontrolerze np. to logowanie - lecisz bezpośrednio do bazy z pominięciem warstwy modelu - może poczytaj coś więcej o MVC to, to zrozumiesz. Zadaniem kontrolera jest tylko odbieranie poleceń od użytkownika i przekazanie ich do modelu czyli logiki aplikacji.

 

Aha tam wcześniej zrobiłem drobny błąd (poprawiłem go od razu, ale może akurat nie odświeżyłeś czy coś i go nie zauważyłeś). Chodzi o to że w tym ostatnim listingu brakowało słowa (zmiennej) model - model.signin(username, password).

---

Dobra. Zrobiłem tak:

Kontroler:

package controller;

import model.*;
import repository.TaskActions;
import repository.UserActions;
import view.ToDoView;

import java.sql.SQLException;
import java.util.InputMismatchException;
import java.util.List;
import java.util.Scanner;

public class ToDoController {

    private static final int LOGIN = 1;
    private static final int REGISTER = 2;
    private UserActions userActions;
    private ToDoView toDoView;
    private Scanner input;
    private int option;
    private Login modelOfLogin;
    private Registration modelOfRegistration;
    private Tasker modelOfTasker;

    public ToDoController(UserActions userStorage, TaskActions taskStorage, ToDoView toDoView) {
        TaskActions taskActions = taskStorage;
        this.userActions = userStorage;
        this.toDoView = toDoView;
        input = new Scanner(System.in);
        modelOfLogin = new Login(userActions);
        modelOfRegistration = new Registration(userActions);
        modelOfTasker = new Tasker(taskActions);
    }

    public void startApplication() {
        toDoView.executeMainMenu();
        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) {
                toDoView.showError();
                input.next();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }

    private void executeUserOptions(int option) throws SQLException {
        switch (option) {
            case 1:
                toDoView.showInputingTaskMessage();
                String taskName = input.next();
                modelOfTasker.addTask(taskName, modelOfLogin.getConnectedUser());
                break;
            case 2:
                toDoView.showInputingTaskMessage();
                taskName = input.next();
                modelOfTasker.deleteTask(taskName, modelOfLogin.getConnectedUser());
                break;
            case 3:
                List<Task> tasks = modelOfTasker.getTasks(modelOfLogin.getConnectedUser());
                for (Task task : tasks)
                    toDoView.showMessage(String.valueOf(task));
                break;
        }
    }

    private void executeUserCase() throws SQLException {
        do {
            toDoView.showTaskMenu();
            option = input.nextInt();
            executeUserOptions(option);
        }
        while (option != 4);
    }

    private void executeRegistrationCase() throws SQLException {
        String username, password;
        toDoView.showInputingUserAndPasswordMessage();
        toDoView.showMessage("User: ");
        username = input.next();
        toDoView.showMessage("Password: ");
        password = input.next();
        if (modelOfRegistration.createUser(username, password))
            toDoView.showMessage("User created successfully, now you can sign in!");
    }

    private void executeLoginCase() throws SQLException {
        String username, password;
        boolean isInvalid = true;
        do {
            toDoView.showInputingUserAndPasswordMessage();
            toDoView.showMessage("User: ");
            username = input.next();
            toDoView.showMessage("Password: ");
            password = input.next();
            if (modelOfLogin.signIn(username, password)) {
                toDoView.showMessage("You've logged in");
                executeUserCase();
                isInvalid = false;
            } else
                toDoView.showError();
        }
        while (isInvalid);
    }
}

Stworzyłem 3 klasy w pakunku model:

Login:

package model;

import repository.UserActions;

import java.sql.SQLException;

public class Login {

    private User connectedUser;
    private UserActions userActions;

    public Login(UserActions userActions) {
        this.userActions = userActions;
    }

    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 User getConnectedUser() {
        return this.connectedUser;
    }
}

Registration

package model;

import repository.UserActions;

import java.sql.SQLException;

public class Registration {
    private UserActions userActions;

    public Registration(UserActions userActions) {
        this.userActions = userActions;
    }

    public boolean createUser(String username, String password) throws SQLException {
        return userActions.createUser(new User(username, password));
    }
}

Tasker

package model;

import repository.TaskActions;

import java.sql.SQLException;
import java.util.List;

public class Tasker {
    private TaskActions taskActions;

    public Tasker(TaskActions taskActions) {
        this.taskActions = taskActions;
    }

    public void addTask(String taskName, User connectedUser) throws SQLException {
        taskActions.addTask(new Task(taskName), connectedUser);
    }

    public void deleteTask(String taskName, User connectedUser) throws SQLException {
        taskActions.deleteTask(new Task(taskName), connectedUser);
    }

    public List<Task> getTasks(User connectedUser) throws SQLException {
        return taskActions.getTasks(connectedUser);
    }
}

---

Tak, teraz wygląda to dużo lepiej.

Jedna rzecz jeszcze rzuciła mi się w oczy mianowicie to:

public ToDoController(UserActions userStorage, TaskActions taskStorage, ToDoView toDoView)

Według mnie mylące może być przekazywanie do kontrolera tych storage-ów. Wydaje mi się że dużo lepiej by to wyglądało np. tak:

public ToDoController(Tasker tasker, Registration registration, Login login, ToDoView toDoView)

I jeszcze nad jedną rzeczą warto by się zastanowić. Bo teraz ten jeden kontroler co masz odpowiada za wszystko tzn. za rejestracje, logowanie i zarządzanie tymi taskami. Czy nie trzeba by zrobić do tego osobnych widoków i kontrolerów?

---

O ile podział kontrolerów jest sensowny, tak widoków nikoniecznie. Znajduje się tam niespełna 5 metod, które są używane w praktycznie każdym kontrolerze, jedynie dwie metody 

executeMainMenu

oraz 

showTaskMenu

są używane jednorazowo, tak inne niekoniecznie. Dlatego raczej bez sensu tworzyć klasę z tymi samymi metodami.

---

W sumie nie chodzi o to ile jest tam metod, tylko bardziej o odpowiedzialność klasy. W tym momencie masz tak ze jeden widok i jeden kontroler odpowiada za wszystko.

Mając rozbite wszystko na np. RegisterView, LoginView, TaskerView + do tego odpowiednie kontrolery już z samej analizy struktury klas (bez zaglądania w kod) można się połapać o co chodzi w projekcie. Mając jeden wspólny widok czy kontroler trzeba go analizować w celu np. znalezienia obsługi rejestracji.

---

Rozumiem. Dzieki.

---

Stworzyłem 3 kontrolery odnośnie rejestracji, logowania i odnośnie tasków.

Wszystko działa, ale w tym sęk, że nie wiem czy to dobrze zrobiłem, bo kod mi się zagmatwał w mainie 10x w porównaniu do poprzedniego.

Main:

import controller.LoginController;
import controller.RegistrationController;
import controller.TaskManagementController;
import controller.ToDoController;
import model.Login;
import model.Registration;
import model.Tasker;
import repository.TaskActions;
import repository.TaskRepository;
import repository.UserRepository;
import view.ToDoView;

import java.sql.SQLException;

public class Main {

    public static void main(String[] args) throws SQLException {
        TaskRepository taskRepository = new TaskRepository();
        UserRepository userRepository = new UserRepository();

        ToDoView toDoView = new ToDoView();

        Login login = new Login(userRepository);
        Registration registration = new Registration(userRepository);
        Tasker tasker = new Tasker(taskRepository);

        LoginController loginController = new LoginController(login, toDoView);
        RegistrationController registrationController = new RegistrationController(registration, toDoView);
        TaskManagementController taskManagementController = new TaskManagementController(tasker,toDoView,loginController);

        ToDoController toDoController = new ToDoController(taskManagementController, loginController, registrationController, toDoView);
        toDoController.startApplication();
    }
}

ToDoController: (zbiera całość) 

package controller;

import view.ToDoView;

import java.sql.SQLException;
import java.util.InputMismatchException;
import java.util.Scanner;

public class ToDoController {

    private static final int LOGIN = 1;
    private static final int REGISTER = 2;
    private ToDoView toDoView;
    private Scanner input;
    private LoginController loginController;
    private RegistrationController registrationController;
    private TaskManagementController taskManagementController;

    public ToDoController(TaskManagementController taskManagementController, LoginController loginController, RegistrationController registrationController, ToDoView toDoView) {
        this.toDoView = toDoView;
        this.taskManagementController = taskManagementController;
        this.loginController = loginController;
        this.registrationController = registrationController;
        input = new Scanner(System.in);
    }

    public void startApplication() {
        toDoView.executeMainMenu();
        boolean isInvalid = true;
        while (isInvalid) {
            try {
                int option = input.nextInt();
                if (option == 1 || option == 2) {
                    isInvalid = false;
                    switch (option) {
                        case LOGIN:
                            loginController.executeLoginCase();
                            taskManagementController.executeUserCase();
                            break;
                        case REGISTER:
                            registrationController.executeRegistrationCase();
                            break;
                    }
                }
            } catch (InputMismatchException e) {
                toDoView.showError();
                input.next();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
}

RegistrationController

package controller;

import model.Login;
import model.Registration;
import view.ToDoView;

import java.sql.SQLException;
import java.util.Scanner;

public class RegistrationController {
    private ToDoView toDoView;
    private Scanner input;
    private Registration modelOfRegistration;

   public  RegistrationController(Registration modelOfRegistration, ToDoView toDoView) {
        this.toDoView = toDoView;
        this.modelOfRegistration = modelOfRegistration;
        input = new Scanner(System.in);
    }
     void executeRegistrationCase() throws SQLException {
        String username, password;
        toDoView.showInputingUserAndPasswordMessage();
        toDoView.showMessage("User: ");
        username = input.next();
        toDoView.showMessage("Password: ");
        password = input.next();
        if (modelOfRegistration.createUser(username, password))
            toDoView.showMessage("User created successfully, now you can sign in!");
    }
}

LoginController

package controller;

import model.Login;
import model.User;
import view.ToDoView;

import java.sql.SQLException;
import java.util.Scanner;

public class LoginController {

    private Login modelOfLogin;
    private ToDoView toDoView;
    private Scanner input;

    public LoginController(Login login, ToDoView toDoView) {
        this.toDoView = toDoView;
        this.modelOfLogin = login;
        input = new Scanner(System.in);
    }

    void executeLoginCase() throws SQLException {
        String username, password;
        boolean isInvalid = true;
        do {
            toDoView.showInputingUserAndPasswordMessage();
            toDoView.showMessage("User: ");
            username = input.next();
            toDoView.showMessage("Password: ");
            password = input.next();
            if (modelOfLogin.signIn(username, password)) {
                toDoView.showMessage("You've logged in");
                isInvalid = false;
            } else
                toDoView.showError();
        }
        while (isInvalid);
    }


    User getConnectedUser() {
        return modelOfLogin.getConnectedUser();
    }

}

Tutaj nie podoba mi się to i nie wiem czy tak powinienem zrobić czy co, a konkretniej chodzi mi o ostatnią metodę, która zwraca mi obiekt Usera, który zaś zwraca obiekt Usera, czyli mam zdublowanie metody.

To jest ta metoda, która się znajduje w modelu logowania:

public User getConnectedUser() {
    return this.connectedUser;
}

Do czego mi jest to potrzebne?

Potrzebne mi jest to w klasie odnośnie Tasków

package controller;

import model.Task;
import model.Tasker;
import view.ToDoView;

import java.sql.SQLException;
import java.util.List;
import java.util.Scanner;

public class TaskManagementController {

    private LoginController loginController;
    private Tasker modelOfTasker;
    private ToDoView toDoView;
    private Scanner input;


    public TaskManagementController(Tasker modelOfTasker, ToDoView toDoView, LoginController loginController) {
        this.modelOfTasker = modelOfTasker;
        this.toDoView = toDoView;
        this.loginController = loginController;
        input = new Scanner(System.in);
    }


    private void executeTaskOptions(int option) throws SQLException {
        switch (option) {
            case 1:
                toDoView.showInputingTaskMessage();
                String taskName = input.next();
                modelOfTasker.addTask(taskName, loginController.getConnectedUser());
                break;
            case 2:
                toDoView.showInputingTaskMessage();
                taskName = input.next();
                modelOfTasker.deleteTask(taskName, loginController.getConnectedUser());
                break;
            case 3:
                List<Task> tasks = modelOfTasker.getTasks(loginController.getConnectedUser());
                for (Task task : tasks)
                    toDoView.showMessage(String.valueOf(task));
                break;
        }
    }

    void executeUserCase() throws SQLException {
        int option;
        do {
            toDoView.showTaskMenu();
            option = input.nextInt();
            executeTaskOptions(option);
        }
        while (option != 4);
    }
}

Jak widzisz muszę przesłać ten sam obiekt Usera, a nie chciałem się odwoływać do modelu Loginu, więc zrobiłem to w kontrolerze. Dobry ruchu? Czy może powinienem się odwołać bezpośrednio do modelu jak to zrobiłem w klasie LoginController.

Podziele jeszcze później ToDoView na 3 kolejne klasy, ale to już jest najmniejszy problem.

---

Co do maina to podziel to na co najmniej metody i już będzie wtedy logiczna całość. Coś w stylu createController, która zwraca dany kontroler.

Na resztę odpowiem później.

 

Co do tego getConnectedUser() - hm... to może twórz ten kontroler później i przekazuj samego użytkownika:

 switch (option) {
   case LOGIN:
       loginController.executeLoginCase();
                      
       TaskManagmentModel tmm = new TaskManagmentModel(loginModel.getConnectedUser() );
       TaskManagmentController taskManagementController = new TaskManagementController(toDoView, tmm);

       taskManagementController.executeUserCase();
      break;

albo ewentualnie, nie przez konstruktor tylko przez setter:

taskManagementController.getModel().setUser(user);

 

---

Chodzi Ci, że np. w klasie Main zrobić 3 metody, które zwracają 3 kontrolery?

czyli:

private LoginController createLoginController(...)
{
...
}

private RegistrationController createRegistrationController(..)
{
...
}

?

Co do tego pobierania obiektu użytkownika nie rozumiem.

Nie mam tak jak napisałeś modelu takiego jak (chyba ze go stworzyc z metoda identyczna jaką dałem w LoginController czyli getConnectedUser): 

TaskManagmentModel tmm = new TaskManagmentModel(loginModel.getConnectedUser() );

chyba, że chodziło Ci o Taskera, ale on przyjmuje całkiem co innego, dlatego nie mam pojęcia o co tutaj chodzi.

Czy pobieranie z modelu logowania byłoby złym pomysłem?

---

Chodzi Ci, że np. w klasie Main zrobić 3 metody, które zwracają 3 kontrolery?

Tak.

 

Tak chodziło o Taskera, staram się pisać ogólnymi pojęciami własnie żebyś lepiej rozumiał.

Czy pobieranie z modelu logowania byłoby złym pomysłem?

Nie wydaje mi się. Skądś to trzeba wziąć więc model, według mnie, jest sensowniejszy niż kontroler czy widok.

---

W takim razie, dodam po prostu model logowania do taska, bo w sumie zarządzanie taskami opiera się na logowaniu, czyli jest od niego zależne.

---

Zadaj sobie tylko pytanie, czy uzależnianie od skomplikowanego modelu LoginModel (skomplikowanego w sensie, że są w nim także operacje z których nie powinno się korzystać w innym miejscu niż przeznaczony do tego kontroler) jest lepsze niż przekazanie prostego obiektu z danymi użytkownika.

---

Zgadzam się, ale rozwiązanie które podałeś, czyli: 

TaskManagmentModel tmm = new TaskManagmentModel(loginModel.getConnectedUser() );
      TaskManagmentController taskManagementController = new TaskManagementController(toDoView, tmm);

robi zamęt w switchu.

Jeżeli chodzi o drugie rozwiązanie czyli:

taskManagementController.getModel().setUser(user);

to tutaj już nie bardzo wiem jak to ma wyglądać w środku.

Poza tym, przekazujesz obiekt usera, a ja w głównym kontrolerze nie mam żadnego obiektu. Wszystko sprowadza się do modelu logowania, bo tam ten obiekt użytkownika jest.

 

---

Mam wrażenie że Ty cały czas operujesz tylko na tym co masz w kodzie i stąd Twoje niezrozumienie. Cały czas staram się mówić ogólnie, a Ty próbujesz to łączyć z tym co masz aktualnie - podkreślam aktualnie - w kodzie (przyznam się, że trochę zaczynam się irytować).

 

MVC- jeszcze raz - Model Widok Kontroler - każdy kontroler ma w sobie Widok i Model więc coś takiego:

taskManagementController.getModel();

Może dawać do zrozumienia że z kontrolera pobieram instancje jego modelu - a jak nazwiesz ten model to już Twoja rzecz.

Popatrz na to bardziej ogólnie.

I w sumie czemu zagmatwa? Bo będzie w nim więcej niż jedna linia? To można to przenieść do jakieś klasy fabrycznej:

class TaskManagmentFactory
{
       public static TaskManagmentController createTaskControllerFor(User currentUser)
        {
               TaskRepo taskRepo = new TaskRepImpl();
               TaskModel taskModel = new TaskModel(taskRepo, currentUser);
               TaskView taskView = new TaskView();

                return new TaskManagmentController(taskView, taskModel);
         }
}

//wtedy w switchu masz prosty kod;

case LOGIN:
   loginModel.login(username, password);
              
   TaskManagmentController tmc = TaskManagmentFactory.createTaskControllerFor(loginModel.getConnectedUser());
   tmc.executeAction();
   break;