Zgadza się. Podniosłem tę kwestię opierając ją na swojej wiedzy i doświadczeniu, co autor z tą uwagą zrobi, to już jego sprawa.
0
0
Wrzuciłem jakieś poprawki, większość rzeczy które zapropnowałeś @Afish wprowadziłem, natomiast miałem wątpliwości z poniższymi. Link oczywiście ten sam, https://github.com/WodorHel/WebSocket-Server :P
Afish napisał(a):
https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ReceivingDataService.cs#L49
To zamień tego ifa na filtr z nowego C#. I od razu pytanie-zagadka: dlaczego filtr, a nie tak, jak jest teraz?
W sumie oprócz skróconego opisu to nie wiem, w przypadku braku zamknięcia klienta i tak exception poszedłby dalej do usera z zachowanych stack tracem. Anyway, w nowej wersji niestety dalej "zjadam" całego SocketDisposedException (bo znowu usunąłem jednego managera klientów i nie mam informacji czy socket jest zamknięty), ale we wszystkich kodach podobnych serwerów widziałem to samo rozwiązanie. Po prostu Microsoft skopał to, że po normalnym zamknięciu socketu przez Socket.Shutdown() i Socket.Close() dzieją się takie rzeczy z tymi wyjątkami i ciężko to ogarnąć.
https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L123
Tu chyba nie ma sprawdzenia, czy klient istnieje.
Sprawdzenie jest w wywoływanej metodzie, więc tak czy siak zwróci null jak nie znajdzie id.
https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L181
Logika z tej metody i wszystkich podobnych – nie wiem, czy to nie powinno być gdzieś indziej. Jakoś mi tutaj średnio pasuje. Może w WebSocketClient?
Z tym był największy problem, cała biblioteka jest niestety tak zaprojektowana że WebSocketClient nie ma dostępu do metod serwera (jest bardziej kontenerem na dane), stąd nie zrobi sobie sama hendszejka ani nie wyśle danych. Na ten moment chyba zostawię tak jak jest, przy ewentualnym przepisywaniu całego kodu inaczej to zaprojektuję.
https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L25
Bufor powinien być liczbą nieujemną.
To + sprawdzenie w kontruktorze i ewentualne wyrzuceniu wyjątku ArgumentOutOfRange. W ogóle to zastanawiam się, czy takie sprawdzanie robić tylko dla parametrów wprowadzanych przez usera libki, czy w każdej klasie w razie gdybym ja się pomylił w czasie pisania.
EDIT: Zabrakło jeszcze sprawdzania numeru portu, ale zapamiętam żeby poprawić.
https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SocketClients/Client.cs#L17
Jakoś nie jestem przekonany, że id jako guid serializowany do stringa jest dobrym pomysłem. Dlaczego nie zwykły integer?
Między innymi dlatego że łatwiej zrobić Guid.NewGuid(), niż pisać osobne klasy do ładnego generowania id. Poza tym wychodzę z założenia, że nie piszę programów na AVR czy inne Arduino i nie muszę walczyć o każdy bajt pamięci :P A taki string trochę zajmuje, to fakt.
Mam jeszcze pytanie dotyczące interfejsów, jak piszę jakieś normalne appki w ASP.NET MVC to wiadomo, jest podział na warstwy, DI, itp. Natomiast czy tutaj jest sens tworzenia interfejsów dla głównych klas typu WebSocketServer, ServerListener itd? Widziałem w kilku projektach takie podejście, ale raz że nie wykorzystuję tutaj żadnego kontenera IoC, a dwa to klasy są ze sobą na tyle związane, że raczej nie ma korzyści płynących z separacji poszczególnych implementacji.
1
EntityPamerano napisał(a):
W sumie oprócz skróconego opisu to nie wiem, w przypadku braku zamknięcia klienta i tak exception poszedłby dalej do usera z zachowanych stack tracem.
Różnica jest bardzo mała, taki filtr nie gubi śladu stosu w przypadku niespełnienia warunku. Jak jesteś zainteresowany, to tutaj szczegóły (uwaga, autopromocja!): https://blog.adamfurmanek.pl/2016/10/01/handling-and-rethrowing-exceptions-in-c/
To + sprawdzenie w kontruktorze i ewentualne wyrzuceniu wyjątku ArgumentOutOfRange. W ogóle to zastanawiam się, czy takie sprawdzanie robić tylko dla parametrów wprowadzanych przez usera libki, czy w każdej klasie w razie gdybym ja się pomylił w czasie pisania.
EDIT: Zabrakło jeszcze sprawdzania numeru portu, ale zapamiętam żeby poprawić.
Moim zdaniem powinieneś takie rzeczy sprawdzać tylko raz, czyli albo po otrzymaniu od użytkownika, albo opakowujesz takie wartości we własne typy i w konstruktorach sprawdzasz poprawność. W zależności od języka/używanej technologii możesz też bawić się w kontrakty lub o wiele mocniejsze typy, ale w C# nie jest to takie wygodne.
Mam jeszcze pytanie dotyczące interfejsów, jak piszę jakieś normalne appki w ASP.NET MVC to wiadomo, jest podział na warstwy, DI, itp. Natomiast czy tutaj jest sens tworzenia interfejsów dla głównych klas typu WebSocketServer, ServerListener itd? Widziałem w kilku projektach takie podejście, ale raz że nie wykorzystuję tutaj żadnego kontenera IoC, a dwa to klasy są ze sobą na tyle związane, że raczej nie ma korzyści płynących z separacji poszczególnych implementacji.
Ja zasadniczo jestem przeciwny wprowadzaniu wszędzie interfejsów z dokładnie jedną implementacją, ale zazwyczaj bez tego trudniej jest przetestować aplikację. Napisz trochę testów jednostkowych, to zaraz zobaczysz, gdzie musisz się nagimnastykować i gdzie warto wprowadzić interfejsy.
0
W sumie testy głównych klas będzie trochę ciężko pisać, zważywszy na to że są ściśle związane z klasą Socket, ale inne klasy powinno dać radę. Artykuł przeczytałem, wszytko jest już jasne :)
0
Wrzuciłem na GitHuba parę testów jednostkowych, nie pokrywają one niestety całego projektu, ale generalnie mam wrażenie że takie libki (zwłaszcza te które opierają się stricte na socketach, plikach itp.) gorzej się testuje niż normalne projekty, zgodne na przykład z MVC, gdzie są jasno wydzielone warstwy.
Projekt: https://github.com/WodorHel/WebSocket-Server/tree/master/WS
Testy: https://github.com/WodorHel/WebSocket-Server/tree/master/Tests
A poza tym, dodałem walidację dla klasy WebSocketServer opartą o kontrakty (Code Contracts) i sporo innych mniejszych poprawek. Chciałem się trochę pobawić Moq i zrobiłem dla kilku klas interfejsy, ale ostatecznie nigdzie nie było mi to potrzebne. Szkoda :|