Console App rewizja kodu

Hej, na GH wrzuciłem małą aplikację konsolową:
https://github.com/przemyslawbak/Trials/tree/master/Console%20App%2001%20Trials/Testy%2016%20Console%20Menu

Ciekaw jestem czy coś byście pozmieniali w niej.

Generalnie chciałem ją utrzymać we wzorcu MVVM. Jest to prosta aplikacja która wykonuje podstawowy CRUD na obiektach z bazy. Jeśli nie ma bazy, to tworzy nową. Jeśli nie ma obiektów w niej, to seeduje ją. Pozostaje mi jeszcze poprawienie metody edycji rekordu i dodanie funkcji dodawania obiektu w modelu. A właśnie, ciekaw jestem jak klasa DatabaseRepository będzie się miało do MVVM według Was?

Proszę Was o opinię, bo do końca nie wiedziałem jak ugryźć aplikację konsolową korzystającą z biblioteki EF pisaną w MVVM, a właśnie taką sobie wymyśliłem.

Pzdr

1. Piszesz za dużo komentarzy, które nic nie wnoszą. Okomentowałeś np. PrintHeader(); //print header, lecz nie wytłumaczyłeś co robi klasa RelayCommand i w jaki sposób może być przydatna.

2. Repozytorium kodu w założeniu nie powinno zawierać żadnych rzeczy, które da się na podstawie kodu zbudować / odtworzyć (dzięki temu jest ono mniejsze oraz znacznie wygodniej pracuje się w kilkuosobowych drużynach) - dlatego nie powinieneś tam trzymać żadnych plików .exe oraz pokrewnych.

3. Wchodząc w Twój główny katalog aplikacji (Trials/Console App 01 Trials/Testy 16 Console Menu) nie widać czym Twoja aplikacja się zajmuje (jest to przykład tzw. architektury nieśmiałej). Spróbuj ułożyć katalogi / klasy zgodnie z ich zastosowaniem (domeną), a nie wzorcami projektowymi ( ).

4. Załóżmy, że chciałbyś przerobić Twoją aplikację tak, aby działała w architekturze klient-serwer. Przez takie fragmenty staje się to niemożliwe. Pomyśl jak przeprojektować aplikację tak, abyś wszystkie odwołania do Console miał tylko w widokach / kontrolerach, a nie porozrzucane losowo po kodzie.

5. https://github.com/przemyslawbak/Trials/blob/master/Console%20App%2001%20Trials/Testy%2016%20Console%20Menu/Models/DataModelValidator.cs#L60 - sprawdzanie formatu nie powinno łączyć się z bazą danych; powinieneś mieć dwa osobne walidatory (IsIdValid + DoesIdExist na przykład).

6. https://github.com/przemyslawbak/Trials/blob/master/Console%20App%2001%20Trials/Testy%2016%20Console%20Menu/Models/DatabaseModel.cs#L9 - DatabaseModel wcale nie opisuje zastosowania tej klasy. Czym ona jest? Co ona opisuje? Jeśli ta klasa modeluje pracownika, powinna nazywać się np. Employee.

7. https://github.com/przemyslawbak/Trials/blob/master/Console%20App%2001%20Trials/Testy%2016%20Console%20Menu/Models/DatabaseRepository.cs#L11 - dane połączeniowe do bazy powinny być konfigurowalne w aplikacji, nie wpisane na sztywno w kod.

8. https://github.com/przemyslawbak/Trials/blob/master/Console%20App%2001%20Trials/Testy%2016%20Console%20Menu/Models/DatabaseRepository.cs#L38 - nazwa CheckForDB jest zbyt generyczna i na podstawie samej nazwy nie da się domyślić, jakiego zachowania / rezultatu można oczekiwać (sprawdza ona czy baza odpowiada na ping? a może sprawdza czy jest połączenie do bazy? a może sprawdza czy w bazie istnieją odpowiednie tabele? [...]). Zauważ dodatkowo, że komentarz check if there is DB already jedynie powtarza to, co już można wydedukować na podstawie nazwy i nie wnosi nic nowego.

Zamiast używać goto powydzielaj te bloki do innej funkcji - klasy.

Patryk27 napisał(a):

(...)

Dzięki za solidną odpowiedź. Świetny komentarz. Co do Twoich uwag:

Ad 1. Postaram się popracować nad tym, żeby nie komentować rzeczy oczywistych.
Ad 2. Też próbuję się oduczyć złych nawyków, których nabrałem z literatury, gdzie wrzucano do repo praktycznie wszystko. Wciąż mam trochę do ogarnięcia jak chodzi o architekturę, ale jest to na liście moich najbliższych "to do".
Ad 3. To akurat było zrobione umyślnie. Zabierając się za aplikację nie wiedziałem czym ona będzie się zajmowała, stąd bardzo ogólne nazwy.
Ad 4. Dobry pomysł, przyznaję że też, jak już miałem trochę kodu naklepanego, zauważyłem że komunikaty konsolowe mogłyby być generowane z jakiejś metody.
Ad 5. Jak dobrze rozumiem, walidator powinien sprawdzać obiekty z bazy przez repo?
Ad 6. To jak w Ad 3, miało być ogólne bardzo.
Ad 7. Ale sugerujesz jakiś konkretny plik w którym takie dane powinny być umieszczone?
Ad 8. Sprawdza czy baza w ogóle istnieje, ale rozumiem, że to kwestia dobrej praktyki nazywać jaśniej metodę.

Ad 3. To akurat było zrobione umyślnie. Zabierając się za aplikację nie wiedziałem czym ona będzie się zajmowała, stąd bardzo ogólne nazwy.

Jest to bardzo zła praktyka; powinieneś najpierw coś zaprojektować, a potem kodować. Przygotuj sobie choćby na kartce najprostszy spis wymagań, wstępny szkic bazy i dopiero mając to wszystko odpalaj IDE. Godzina projektowania potrafi zaoszczędzić tygodnie kodowania ;-)

Ad 5. Jak dobrze rozumiem, walidator powinien sprawdzać obiekty z bazy przez repo?

Tak.

Ad 7. Ale sugerujesz jakiś konkretny plik w którym takie dane powinny być umieszczone?

Niestety nie jestem pewien, jakie są przyjęte standardy w przypadku C#; na sam start wystarczyłby pewnie jakikolwiek inny plik ze stałymi.