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 :)
w Visual Studio jest narzędzie: testy jednostkowe... zamiast szukać pomocy prawdopodobnie bezskutecznie może warto byłoby zapoznać się z tym narzędziem... jest też w książce J.Matulewski Helion VS-2013 opisane za jedyne chyba 30 PLN ebooka se kupić można...
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
Dzięki za odpowiedź ;) Zaraz się zabieram do poprawiania. Książkę na razie muszę odłożyć sobie do listy "do przeczytania", jeszcze nie skończyłem poprzednich.
nie ma za co. co do ksiazki ktora podalam to polecam na poczatek (jak juz znajdziesz czas), tak zeby poznac podstawy platformy, procentuje przy nauce innych rzeczy i ogolnie w codziennej praktyce.
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
?
wystarczy
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?
Jest to dość często spotykana konwencja nazewnictwa pól klasy (zakładam, że to je nazwałeś "zmiennymi prywatnymi").
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ę"?
Dobra, ogarnąłem mniej więcej te Bindingi, jutro zacznę przerabiać program :) Myślałem już, że nigdy się nie kapnę o co chodzi :D
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.