Serwer komunikatora. Wskazówki i ocena kodu.

Witam. Na wstępie chcę powiedzieć, że głównie chodzi mi o styl pisania takiej aplikacji i chciałbym usłyszeć główne zarzuty co do dotychczasowego kodu, aczkolwiek drobniejsze jakieś usterki, dziwactwa proszę również uwzględnić. Uprzedzam też o tym, że nie musisz tego sprawdzać jak ci się nie chcę, szukam kogoś kto miałby wolną chwilkę, bo dla Was ekspertów to 5 min, przeczytać ten opis, spojrzeć na kod i ocenić jego "poprawność".

No to do rzeczy. Od jakiegoś miesiąca zacząłem pisać komunikator (na razie serwer, na klienta przyjdzie czas oczywiście). Nie mogłem wymyślić nic innego, więc postanowiłem właśnie komunikator, dobre ćwiczenie na wielowątkowość, komunikacja sieciowa, bazy danych i przeróżne inności. Jestem na takim etapie, że przydałoby się przemyśleć jak takie działanie serwera ma wyglądać, żeby było szybko, z możliwością ciągłego rozwijania i ładny kod. Na razie wygląda to tak:

• główny wątek - inicjalizacja, łączenie z bazą, itd.
• wątek nasłuchiwania - nasłuchuje, jak jest nowy klient to go obsługuje: przyłącza tymczasowo; odbiera żądanie; wykonuje żądanie lub nie
• wątek "pracy" - na razie tylko przelatuje wszystkich klientów online (tych przyłączonych do listy) i odbiera od nich żądania
• lista klientów - przechowuje klientów online
• klasa Account - odpowiedzialna za tworzenie kont, sprawdzanie itd.
• klasa Utils - logi, obsługa bazy i inne
• statyczna klasa Cmd - zawiera komendy swoje i klienta

Prócz tego zasada działania metod z różnych klas jest taka, że każda metoda zwraca mój enumError. Jest kilka stanów, w zależności od metody np. ta od tworzenia konta zwraca: ServerError (to w sumie każda zwraca, bo zawsze jakieś ryzyko błędu innego jest), AccountExists (nie można stworzyć II konta o takim samym loginie, [błąd bazy]), NoError (też prawie każda zwraca, gdy wszystko ok, czyli konto utworzone). Przy wywołaniu takiej metody mam switch'a i w zależności od wyniku wykonuje różne operacje.

Tyle opisu. Załączam w całości kod. Wiem, że opis duży, kodu w sumie nie wiele ~560 linii odjąć jak wiadomo usingi i wolne linie dla czytelnego kodu. Jeżeli ktoś na prawdę by miał czas popatrzeć na to, nie mówię już o dogłębnej analizie, tylko spojrzeć czy dobrze myślę i czy w pracy by mnie za taki kod nie wywalili :)

Miałem wrzucić do "Oceny i recenzje", ale chyba się nie nadaje, bo to nie gotowy projekt, jak to można nazwać. Ogólnie to pierwsza rzecz, którą chcę bardzo porządnie napisać i zapamiętać sobie czy nawet wejść w nawyk pisania dobrego takiej np. aplikacji. Wiem, że sposobów jest setka, ale każde są do siebie w jakiś sposób podobne. Proszę więc o pomoc i cenne wskazówki szanowni koledzy :) I tak na koniec, niech każdy powie ile facepalm'ów zaliczył czytając kod :D Na prawdę, możecie się śmiać, jak zaczynałem miałem tylko mniej-więcej pomysł jak napisać ogólnie, ale żeby tak fachowo i poprawnie to nie, bo nie wiem jak to się robi, właśnie piszę, żeby się dowiedzieć. Acha, no i kod jest otulony niezbędnymi komentarzami.

-- Kod --
http://xeosite.yoyo.pl/Download/XMServer.zip

Kurczę, dlaczego nie można zrobić drzewka z punktatorów? Jak dam niżej to jaja są totalne, a jak dam samo "-", to jest pusta linia przed właściwym punktem..

Szczerze mówiąc? Dość przeciętnie. Słabe wykorzystanie .NET Frameworka (implementowanie własnego wyszukiwania na liście chociażby), o tym, że można tworzyć własne wyjątki w ogóle zapomniałeś. Niektórzy używają ich aż nadto (nie służą one do kontrolowania przepływu programu), ale jeden enum z błędami na cały program brzmi równie niedorzecznie. Brak dokumentacji, brak testów. Przedpotopowy sposób korzystania z baz danych (jedna funkcja w jakiejś ogólnej klasie pomocniczej?). Zaimplementowanie IEnumerable<T> nie służy po to, żeby ręcznie pobierać enumerator, tylko przejechać foreachem. Thread.Sleep(50); w przetwarzaniu asynchronicznym w języku wysokopoziomowym też średnio mi pasuje.

"Brak dokumentacji, brak testów" - wydaje mi się, że na tym etapie mi to nie potrzebne, testować testuję, jak możliwie najdokładniej wszystkie funkcje. Bardzo dobrze, że dość przeciętnie nie oczekiwałem ocen nie wiadomo jakich. Zdaję sobie sprawę, że kod nie jest dobry, bo jak wspomniałem, uczę się jak każdy, wiadomo nikt od razu kodu zajebistego nie napisał. Co do sposobu korzystania z bazy, jak mam go rozszerzyć? W sensie takim, że jeszcze oddzielna klasa i tam metody typu: SelectRow, CreateTable itd.? A jak inaczej z IEnumerable zrobić, by mieć dostęp do wszystkich elementów w liście, która jest z kolei w mojej klasie, z zewnątrz? To miałem na myśli. No i ostatni Thread.Sleep(50);, dałem dlatego, bo podczas ciągłego debugowania, jakby tak te pętle napierd**** to niezbyt by se podebugował :) Ale w sumie i tak po co akurat takie małemu serwerowi napierdzielanie tak co chwila? Dziękuję ci za odpowiedź, tylko żebyś jeszcze mi na tego posta odpowiedział. Liczę też dalej na innych :)

EDIT: Co do IEnumerable...aaa chodzi ci o ten w 'work' threadzie? Tak, racja, lepiej użyć foreacha, ale wtedy nie mam możliwości usunięcia z listy klienta, który chce się odłączyć. Miałem plan dodawać do kolejnej listy usunięte klienty i przed następną pętlą wyrzucić z listy właśnie te, ale wydało mi się lepszy rozwiązaniem zadeklarować iterator.

Dokumentację na poziomie kodu lepiej pisać razem z nim (masz dokumentację xml w komentarzach). Mam nadzieję, że wiesz, że testowanie funkcji / klas to nie jest stworzenie jej instancji w main, wywołanie dwóch metod, skompilowanie i wywalenie kodu. Pisz testy jednostkowe, które będziesz okresowo uruchamiał. TDD w ogóle polega na tym, żeby najpierw napisać testy, a później kod. Czym projekt robi się większy, tym więcej zależności pomiędzy poszczególnymi komponentami, testowanie robi się trudniejsze. Stosuj interfejsy, spójrz na zagadnienia takie jak depedency injection, mockup objects. Z dobrymi testami nie będziesz musiał wstawiać jakiś sztucznych Thread.Sleep.
Nie musisz pisać własnego ORM, ale tworzenie zapytań przez StringBuilder to zły pomysł. Spójrz na dostępne rozwiązania.

A jak inaczej z IEnumerable zrobić, by mieć dostęp do wszystkich elementów w liście, która jest z kolei w mojej klasie, z zewnątrz?

foreach

Ok. To co proponowałbyś zamiast zwracania przez metody enumów? Wyjątki będą lepszym rozwiązaniem? Wtedy switch'e odpadną, ale dojdą try...catch.

Całe to ClientList jest kompletnie niepotrzebne, lepiej dać Dictionary<string,Client> albo Hashmap.

Zawsze gdy piszesz tego typu kod

        public void Add(Client client)
        {
            clients.Add(client);
        }
        public void Remove(Client client)
        {
            clients.Remove(client);
        }

powinna ci się zaświecić czerwona lampka, że tworzysz WTF.

Ten sam efekt uzyskasz dziedzicząc po List<Client> i dodając jedną funkcję:

    class ClientList : List<Client>
    {
        public Client this[string login]
        {
            // return client if there exists a client with that login
            // else return null
            get
            {
                foreach (Client client in this)
                    if (client.Login == login)
                        return client;
                return null;
            }
        }
    }

ale, jak już powiedziałem, lepiej używać gotowych kontenerów.