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

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;