Witam,
naskrobałem sobie coś takiego https://github.com/pixellos/TCPConnector i chciałbym się dowiedzieć czy:
-dobrze wykorzystuję wzorzec MVVM
-czy klasy są logicznie zbudowane, poprawnie
-coś o komentarzach, czy dobrze opisuję itp
-czy obiektowość jest dobra, pomyślałem, że zamiast kilku parametrów w metodach mogę zrobić klasę, która będzie je przechowywała i nimi zarządzała - dobre podejście?
I co wam tam przyjdzie do głowy :)
Ale mi nie chodzi o "Nie działa, weźcie napiszcie za mnie lepiej, bo tak", tylko raczej
"Napisałem, sprawdziłem, działa, postarałem się obsłużyć wyjątki, napisać najlepszy kod na miarę moich możliwości i proszę o powiedzenie co mógłbym zrobić lepiej" ;)
Bardziej niż o techniczne sprawy chodzi mi o strukturę kodu, czy dobrze podzieliłem projekt na MVVM, czy kod jest w miarę logicznie nazwany, zrozumiały itp :)
tak przeklikalam przez pare plikow, uwagi (skupie sie na wadach bo chyba to cie interesuje, nie?):
wpf'a nie znam wiec sie nie wypowiem
Pixello napisał(a):
pomyślałem, że zamiast kilku parametrów w metodach mogę zrobić klasę, która będzie je przechowywała i nimi zarządzała - dobre podejście?
I co wam tam przyjdzie do głowy :)
czasem tak, czesciej zdecydowanie nie bo tworzysz sobie globalny god object ktorego skomplikowanie i zabugowanie bedzie roslo wykladniczo w miare rozwoju projektu. postaraj sie minimalizowac ilosc parametrow i odpowiedzialnosc kazdej klasy/metody. jesli widzisz ze dana rzecz robi zbyt wiele rzeczy, to znak ze nalezy ja podzielic na mniejsze, bardziej wyspecjalizowane.
edit: dobra ksiazka do przeczytania dla ciebie - http://www.amazon.com/Essential-Edition-Microsoft-Windows-Development/dp/0321877586
wystarczy
C# IsConnected() ?? false
np. if(IsConnected() ?? false)
{
//ok, polaczony, tu cos robimy
}
https://msdn.microsoft.com/en-us/library/ms173224.aspx
edit: odpowiadajac na pytanie, 2 sposob jest lepszy (i dziala), ale porownywanie z true/false jest brzydkie, w przykladzie ktory ci dalam mozesz dodatkowo zdefiniowac co jest defaultem (w moim przypadku false)
Common_Files - nazwa bez sensu, bo przestrzenie nazw zawierają klasy, nie pliki.abstract_Connector - po co ten abstract_ w nazwie?ChangeParametrs - to nie lepiej po prostu zrobić nowy obiekt, skoro ma robić coś innego?Common mieszasz "silnik" swojej aplikacji z funkcjami pomocniczymi dla GUI? To jest bałagan... Bałagan, który zawsze tworzy się w projektach o nazwie Common. :)Pixello napisał(a):
Jeszcze mam pytanko, może bardziej do działu newbie pasuje, ale jak w ifach sprawdzam wartość, która jest bool?, to lepiej
IsConnected().Value.Equals(true);
czy
IsConnected() == true
?
Jeśli IsConnected() zwraca Nullable<bool>, to myślę, że lepiej napisać if(connected.HasValue && connected.Value) (oczywiście wydzielając wcześniej zmienną, a nie pisać jakieś spaghetti w ifie), bo od razu widać o co chodzi i dlaczego.
Pytanie zasadnicze, czemu IsConnected zwraca taki typ - przecież albo jest się połączonym, albo nie jest, po prostu nie ma trzeciej możliwości. A jak chcesz tworzyć trzeci możliwy stan, to powinieneś zrobić enum.
W zamyśle miało zwracać null, gdy chodź jeden z obiektów jest niezainicjowany, potem zmieniłem na zawsze false oprócz sytuacji gdzie jest wszystko ok. Rozumiem, że 1 plik - 1 klasa? Zaraz wyodrębnię metody pomocnicze dla GUI do innego pliku. To, że zmieniam parametry zamiast tworzenia nowego obiektu to jakiś wielki błąd?
-dobrze wykorzystuję wzorzec MVVM?
Nie.
Przekazujesz jakieś labele z VM do View, do tego wykorzystujesz ButtonClicked i TextChanged w code behind.
W code behind powinieneś co najwyżej ustawić DataContext i to powinien być koniec grzebania w tym pliku. Zamiast ButtonClicked możesz zbindować się do Command, zamiast TextChanged nie pamiętam do czego się bindowało, pogogluj.
W MVVM ViewModel powinien jedynie udostępniać dane dla widoku, nie sam je modyfikować. A w widoku wyświetlasz je poprzez bindowanie.
Pixello napisał(a):
W zamyśle miało zwracać null, gdy chodź jeden z obiektów jest niezainicjowany
Czy gdybyś pracował w sklepie, i ktoś by Cię spytał, czy masz chleb, to odpowiedziałbyś: "sprzątaczka dzisiaj nie przyszła"?
Bo to mniej więcej robi ta metoda obecnie. Klienta klasy nie obchodzi jej wewnętrzna implementacja, co on sobie ma zrobić z informacją "obiekt nie działa" w sytuacji gdy pyta o stan połączenia? Po prostu inicjalizuj wszystkie obiekty i nie pisz cieknącego kodu.
BTW: http://lambda-the-ultimate.org/node/3186
Rozumiem, że 1 plik - 1 klasa?
No raczej.
Zaraz wyodrębnię metody pomocnicze dla GUI do innego pliku.
Nie pliku tylko klasy. I trzymaj ją w projekcie GUI, albo pomocniczym GUI, a nie z rdzeniem aplikacji.
To, że zmieniam parametry zamiast tworzenia nowego obiektu to jakiś wielki błąd?
W ten sposób wprowadzasz niepotrzebną mutowalność, której efektem będzie najprawdopodobniej więcej dziwnych błędów wynikającej z przypadkowej zmiany stanu obiektu. Kod trzeba pisać tak, żeby błędów było jak najmniej.
No i w tej sytuacji trudno to nazwać programowaniem obiektowym, bo dla Ciebie klasa to jedynie zbiór funkcji.
Z grubsza zacząłem poprawiać kod względem tego co napisaliście, szukałem trochę informacji na blogach i SO i zauważyłem, że bardzo często zmienne prywatne zapisują _zmienna, czy to jest jakaś przyjęta konwencja u "lepszych" programistów?
Mnie się jeszcze mocno C trzyma, dlatego ciężko mi się przyzwyczaić do tej obiektowości, ale się staram. Używacie czegoś takiego jak PDSA developer units (prowadzący w tucie microsoftu używał)? Poczytałem trochę więcej, jak tworzyć prawdziwy MVVM w WPF, i troszkę się przeraziłem ile to dodatkowego kodu ;) Co sądzicie o bibliotekach typu MVVM light/Caliburn.Micro, spróbować jej użyć, czy na początek uczyć się "na piechotę"?
O takie coś chodziło? https://github.com/pixellos/TCPConnector/tree/MVVM
Wywaliłem cały code-behind z widoków, pobindowałem do ICommand oraz do właściwości, odświeżanie odbioru przez BackgroundWorkera, reszta kodu mały regress zaliczyła, ale będę dalej pracował nad nim.
@Edit
Poprawiłem kod, dodałem transmisję w obie strony, starałem się poprawić kod i zastosować wzorzec MVVM.
Mógłby ktoś zerknąć, czy jest lepiej według was?
https://github.com/pixellos/TCPConnector
Tak mniej więcej teraz wygląda, nie wywala przy co drugiej akcji. Jakbyście mogli przejrzeć i powiedzieć o tym kilka słów byłbym wdzięczny :)
Chciałbym się dowiedzieć, czy jest lepiej, starałem się dobrze nazywać funkcje i zmienne, pisać kod czytelny itp.
Jeśli chodzi o wzorzec MVVM to teraz jest dużo lepiej. Wywal może tylko te nieużywane eventy w MainWindow.
:ES i :AYT? nie wiadomo co to jest,
Mogę też zasugerować przeniesienie defaultowych tekstów z textboxów np "127.0.0.1" do settingsów. Dzięki temu przy otwieraniu okna będziesz miał ostatnią wartość którą tam wpisałeś przy poprzednim uruchomieniu.
Zamiast:
public bool IsStreamDescribed()
{
if (newStream == null)
{
return false;
}
else return true;
}
Możesz napisać tak:
public bool IsStreamDescribed()
{
return newStream != null;
}
Zwróciłbym też uwagę na nazwy plików np. tutaj: https://github.com/pixellos/TCPConnector/blob/master/Common%20Files/IActionClass.cs
Nazwa pliku sugeruje, że to interfejs, a w środku jest klasa(z inną nazwą).
Kiedyś był tam interfejs i tak zostało :D Zaraz spróbuję coś z tym zrobić. Dzięki wam za odpowiedzi :).
Zmieniłem IsStreamDescribed na właściwość z akcesorem get.
@dam1an, mógłbyś rozwinąć? Nie rozumiem jak mógłbym przechowywać wartość z poprzedniego uruchomienia w właściwości.
HasValue, a nie porównywanie znull.