kmph
2018-09-03 15:45

Follow-up do ostatniego postu w temacie przechowywania rozgrywek #mon w słowniku w pamięci.

Dobrze - przekonaliście mnie - zamierzam pozbyć się słownika w pamięci, wszystkie rozgrywki - tak trwające, jak i minione - przechowywać w bazie danych. Niemniej, jak wiele innych poprawek, roboczo odłożyłem sobie to zadanie na później, tj. do czasu, gdy skończę prototypować zaplanowane funkcjonalności.

No a jedną z tych funkcjonalności była obsługa replayów oraz pozwolenie na obserwowanie rozgrywki, w której nie bierze się udziału. W tym celu, myślę sobie, stan gry powinien być blokowany w jakiś mądrzejszy sposób, aniżeli lock - wydawało mi się, że to jest typowy przypadek czytelników i pisarzy, więc postarałem się użyć ReaderWriterLockSlim. Myślałem, że implementacja tego będzie szybsza, niż od razu pozbycie się słownika i oparcie się tylko o bazę danych. Chyba jednak się myliłem.

Oto (w dużym skrócie) kod BattleState i BattleManager, jaki mi powstał. Kod ten jest niepoprawny, o czym dowiedziałem się już po napisaniu go :(

public class BattleManager : IBattleManager
{
    private ulong next_id = 0;
    private readonly object next_id_lock = new object();
 
    public ConcurrentDictionary<ulong, BattleState> battles = new ConcurrentDictionary<ulong, BattleState>();
 
    public BattleMetaInfo StartBattle(Dictionary<UserInfo, Team> players)
    {
        ulong id;
        lock(next_id_lock)
            curr_id = next_id++;
        var battleState = new BattleState(players, curr_id, this);
        var ret = new BattleMetaInfo(battleState);
        battles[curr_id] = battleState;
        return ret;
    }
 
    public BattleTurnInfo SendCommand(ulong battleId, UserInfo sender, Move command)
    {
        BattleState battleState;
        try {
            battleState = battles[battleId];
            return battleState.update(sender, command);
        } catch(KeyNotFoundException) { return null; }
    }
 
    // Ktoś chce obserwować rozgrywkę, trzeba mu wysłać wszystko, co stało się aż dotąd.
    public BattleTurnInfo GetProgress(ulong battleId)
    {
        BattleState battleState;
        try {
            battleState = battles[battleId];
            return battleState.getUpdates();
        } catch(KeyNotFoundException) { return null; }
    }
 
    // Ktoś chce zobaczyć, jakie rozgrywki obecnie trwają
    public List<BattleMetaInfo> GetOngoingBattles() =>
        battles.Values.Select(b => b.getMetaInfo()).Where(metaInfo => metaInfo != null).ToList();
 
    // Ktoś kliknął na nick innego gracza
    public List<BattleMetaInfo> GetPlayersBattles(string nick) =>
        GetOngoingBattles().Where(metaInfo => metaInfo.players.Contains(nick)).ToList();
 
    // Ktoś wpisał w searchboxa ID rozgrywski
    public BattleMetaInfo GetBattleById(ulong id)
    {
        BattleState battleState;
        battles.TryGetValue(id, out battleState);
        return battleState?.getMetaInfo();
    }
}
 
public class BattleState
{
    private ReaderWriterLockSlim rwlock = new ReaderWriterLockSlim();
    private bool isBeingRemoved = false;
    private readonly BattleManager battleManager;
    private Timer timer = new Timer() { Interval = 120000, AutoReset = false };
 
    public BattleState(Dictionary<UserInfo, Team _players, ulong _battleId, BattleManager _battleManager)
    {
        timer.Elapsed += (sender, e) => timerRunOut(sender, e);
        timer.Start();
        // .. i więcej kodu tu nieistotnego
    }
 
    public void timerRunOut(object sender, ElapsedEventArgs e)
    {
        try
        {
            rwlock.EnterWriteLock();
            // Zakańczamy grę i ogłaszamy przegranym tego, czyja jest teraz kolejka
        }
        catch (ObjectDisposedException) { }
        finally
        {
            try { rwlock.ExitWriteLock(); } catch { }
            _ = removeBattleAsync();
        }
    }
 
    public BattleTurnInfo update(UserInfo sender, Move command)
    {
        bool mustBeRemoved = false;
        try
        {
            rwlock.EnterWriteLock();
            // Dokonujemy odpowiednich zmian i jeśli ten ruch zakańcza rozgrywkę, to mustBeRemoved jest true
        } catch (ObjectDisposedException) { return null; }
        finally
        {
            try { rwlock.ExitWriteLock(); } catch { }
            if (mustBeRemoved) _ = removeBattleAsync();
        }
    }
 
    public BattleTurnInfo getProgress()
    {
        try
        {
            rwlock.EnterReadLock();
            return new BattleTurnInfo(this);
        } catch( ObjectDisposedException) { return null; }
        finally
        {
            try { rwlock.ExitReadLock(); } catch { }
        }
    }
 
    public BattleMetaInfo getMetaInfo()
    {
        try
        {
            rwlock.EnterReadLock();
            return new BattleMetaInfo(this);
        } catch( ObjectDisposedException) { return null; }
        finally
        {
            try { rwlock.ExitReadLock(); } catch { }
        }
    }
 
    public async Task removeBattleAsync()
    {
        lock(this)
            if (isBeingRemoved) return;
            else isBeingRemoved = true;
 
        try
        {
            using (var scope = battleManager.scopeFactory.CreateScope())
            {
                var db = scope.ServiceProvider.GetRequiredService<ApplicationDbContext>();
                var replay = new Replay(battleId, this, DateTime.Now.ToUniversalTime());
                await db.replays.AddAsync(replay);
                await db.SaveChangesAsync();
            }
        }
        finally
        {
            ((IDictionary<ulong, BattleState>)battleManager.battles).Remove(battleId);
            timer.Dispose();
            rwlock.Dispose();
        }
    }
}

Te wszystkie catch (ObjectDisposedException) są po to, żeby chronić się przed sytuacją np. taką: wątek 1 to żądanie przesłania historii rozgrywki (ktoś wołał GetProgress), ale wątek 1 czeka, bo w międzyczasie wątek 2 ma WriteLock i właśnie zakańcza rozgrywkę. Nie jest deterministyczne, kiedy wątek 1 się dostanie - może się najpierw odpalić Dispose. Ale wtedy po prostu zwracamy null i caller (hub SignalR) wie, że ma zamiast tego szukać już w bazie danych. (Bardziej prawdopodobny przykład: żądanie GetProgress przychodzi tuż przed usunięciem bitwy ze słownika. Przeplot: Wątek 1 wyciąga bitwę ze słownika i posiada referencję do niej, wątek 2 odpala ((IDictionary<ulong, BattleState>)battleManager.battles).Remove(battleId); oraz rwlock.Dispose, wątek 1 próbuje założyć ReadLock).

Ale to jest też powód, dla którego powyższy kod jest niepoprawny. Już po napisaniu go przeczytałem, że według oficjalnej dokumentacji ReaderWriterLockSlim.Dispose() rzuca wyjątkami, jeśli są jakiekolwiek wątki, czekające na otrzymanie locka. Jak ja mam zapewnić, że rwlock.Dispose() wykona się dość późno? Tak późno, że żaden wątek już nie czeka ani czekać nie będzie na blokadę? Sytuacja wydaje mi się beznadziejna i dlatego chyba należy ten kod wyrzucić do kosza. Nie mogę tu użyć ReaderWriterLockSlim.

@Afish napisał:

Odnośnie blokad i Dispose - nie widziałem kodu, ale Twój opis sugeruje, że robisz coś bardzo źle.

OK - co tu robię bardzo źle? Pytanie w zasadzie teoretyczne, chcę po prostu się czegoś nauczyć, bo jak napisałem wyżej, nie widzę możliwości wykorzystania tego kodu.

Chyba, że... Jak bardzo złe byłoby rozwiązanie, w którym wołam ReaderWriterLockSlim.Dispose() w destruktorze BattleState? Na Stacku czytam, że Garbage Collector się tu nie myli i nie zawoła tego za wcześnie. Czy to rozwiązanie jest z gatunku tych złych, czy tych tragicznych? Wiem, że to będzie koniecznie wymagało zmiany prędzej czy później, ale może umożliwi mi to doprowadzenie jak najszybciej kodu do działania, bym mógł w międzyczasie zająć się prototypowaniem kolejnych funkcjonalności...

cs

Czegoś nie rozumiem, jeśli EnterWriteLock w bloku try rzuci wyjątkiem, co może znaczyć, że nie mógł zająć blokady, to obowiązkowe zwolnienie w bloku finally jest bez sensu, tzn. nie wiem czy można bezkarnie zdejmować nieistniejącą blokadę w lock'u. Po drugie czy EnterWriteLock może w ogóle rzucić wyjątkiem? Po trzecie nie rozumiem "bo po wzięciu locka wątek może polecieć wyjątek i finally się nie wykona", jeśli finally się nie wykona to chyba najmniejszym problemem jest wtedy deadlock, Po czwarte zamiast twardo zajmować WriteLock można spróbować przez TryEnterWriteLock z timeoutem.

Afish
  1. Zazwyczaj nie można bezkarnie zdejmować blokady, ale można zapytać locka, czy udało się wziąć blokadę.
  2. Tak.
  3. Jeżeli masz sensownie napisaną aplikację, to ten wyjątek jest złapany gdzieś wyżej i zalogowany, aplikacja działa dalej, ale jest problem, bo blokada jest wzięta i nic już jej nie zwolni.
  4. A to swoją drogą, że nigdy nie powinno się brać blokady bez timeouta, ale to nie ma znaczenia w tej dyskusji.
kmph
2018-08-28 01:54

Ostatnio opisywałem pierdółkę. Teraz jednak mam problem IMO poważniejszy. Chociaż mam działające rozwiązanie, jednak na >90% jest to rozwiązanie dalece nieoptymalne. Nie wiem, jakie rozwiązanie jest prawidłowe. Kwestia związana jest mocno z architekturą całego projetku, tym samym post będzie dłuższy.

Problem wydaje się być związany z moją niedostateczną znajomością tego - jak to się nazywa? - "stacka techonologicznego"? - którego używam, tj. C# + .NET Core (w tym Entity Framework, SignalR itd).

Gra jest przez sieć. Pojedyncza rozgrywka to jest 1 na 1 gracz. Gra jest turowa, przy czym tury niekoniecznie muszą być na przemian (czasem ktoś ma 5 tur pod rząd, czasem obaj mają turę jednocześnie - wtedy tura zaczyna się dopiero, gdy obaj wydadzą polecenia.)

Architektura gierki, z jaką zaczynałem, jest następująca: Mam sobie klasę BattleState, która zawiera trwającą rozgrywkę. Mam też klasę BattleManager, która zarejestrowana jest jako Singleton Service. BattleManager zawiera ConcurrentDictionary, który mapuje long Id do BattleState. Gdy ktoś w przeglądarce wyklikuje sobie, jak ma wyglądać jego tura, to te polecenia odbierane są (wraz z Id bitwy) przez huba SignalR, który zależy (na mocy wymuszonego przez framework dependency injection) od BattleManagera. Tenże BattleManager ma metodę public BattleTurnInfo SendCommand(ulong battleId, UserInfo player, Move command), którą wywołuje hub. Wówczas BattleState o tym Id jest wyciągany z ConcurrentDictionary, blokowany, i o ile jest to możliwe (np. nie czekamy wciąż na ruch przeciwnika), to popychamy grę do przodu o 1 turę i zwracamy hubowi postępy w rozgrywce za pomocą BattleTurnInfo (jednocześnie usuwając grę ze słownika, o ile to jest już koniec gry i mamy zwycięzcę). Hub zaś owe postępy rozgłasza po obu graczach (temu, który się ruszył, i jego przeciwnikowi).

Przypuszczam, że to już idzie w poprzek zastosowaniom, do jakich przewidziany jest framework. Przypuszczam, że teoretycznie powinno to wyglądać raczej tak: Gdy tylko ktoś się rusza, to z bazy danych wyciągany jest (i blokowany!) wiersz o Id danej rozgrywki; stan gry jest deserializowany do obiektu BattleState, gra jest popychana naprzód o ile to możliwe, hub rozgłasza postępy o ile ma co rozgłaszać, tymczasem BattleState jest ponownie serializowany i zapisywany do bazy danych. Nie ma żadnych Singletonów, nie ma żadnych ConcurrentDictionary'ów, które udają bazę danych: jest tylko baza danych i wszystkie niezbędne Services są albo Scoped, albo Transient.

Czemu więc nie zrobiłem tego w ten sposób? Z dwóch powodów, jeden był ideologiczny, a drugi praktyczny:

  1. Ideologiczny: Takie wykorzystanie bazy danych wydaje mi się być dziwne. Wydaje mi się to być używanie bazy danych tak, jakby to był RAM. Baza danych to dla mnie jest mechanizm persystencji danych, tj. używamy jej raczej w zastępstwie dysku twardego, nie RAMu: czyli nie zapisujemy i nie wyciągamy z bazy w kółko tych samych wierszy, tylko zapisujemy coś, gdy skończyliśmy na tym operować i odkładamy to na później, by nie zaśmiecały RAMu i by przetrwały ubicie i ponowne uruchomienie aplikacji. Zatem - jak ja to widzę - zapisywanie do bazy trwających rozgrywek nie ma sensu, bo wedle wszelkiego prawdopodobieństwa za chwilę będziemy musieli wyciągać je z powrotem. (Przypuszczalnie nie tak się sprawy mają?) (Na marginesie - gdyby gra była realtime, to już naprawdę nie widzę, jakie inne rozwiązanie niż moje powinno być zastosowane - no może oprócz użycia Unity3D).
  2. Praktyczny: Entity Framework NIE MA SelectForUpdate, tak jak ma Django - w takim razie, nawet pomijając opisaną wyżej ideologię, pojęcia nie mam, jak to zaimplementować. Naprawdę wydaje mi się, że MUSZĘ zablokować rozgrywkę i czekać na jej odblokowanie

OK, działa. Aż do czasu, gdy przychodzi pora na zaimplementowanie timera. Uznałem to za niezbędne, bo inaczej niektóre bitwy mogłyby w ogóle nigdy nie opuścić dicta (tj kiedy któryś gracz w trakcie bitwy się rozłącza i nigdy nie wraca). Timera wyobrażałem sobie tak: Jeśli zaczyna się teraz kolejka gracza A, to zaczynamy liczyć mu 2 minuty na zrobienie tej kolejki. Jeśli się nie zmieści, to natychmiast zakańczamy grę i uznajemy go za przegranego.

Tutaj, z racji braku wiedzy, w ogóle nie wiedziałem, jak się za to zabrać. Uciekłem się do zadania pytania na Stacku. Tam zaproponowano mi na początku zrobienie jakiegoś "housekeeping task" do tego celu, później jednak stwierdzili, że do takich timerów jest to jednak nienajlepszy pomysł. Zamiast tego zaproponowano mi użycie klasy System.Timers.Timer.

Tak też uczyniłem. BattleState zawiera sobie pole typu System.Timers.Timer i kiedy ktoś się ruszy, to dodatkowo: (a) Zatrzymujemy jego timer; (b) Po popchnięciu gry naprzód nastawiamy timer na kolejne 2 min. (I fakt ten rozgłaszamy po graczach w ramach BattleTurnInfo zwracanego przez SendComman. No ale dobrze, teraz ktoś za długo czekał, Timer się aktywuje. Więc timer woła odpowiednią metodę na BattleState, która blokuje się, zakańcza rozgrywkę i woła BattleManager, by usunął ją z dicta.... no i uff, mamy problem - to bo teraz trzeba jakoś rozgłosić koniec gry. Przynajmniej tego drugiego poinformować, że wygrał.

Szukanie po Stacku doprowadziło w końcu do rozwiązania, które działa, ale obawiam się, że i tak jest błędne. Mianowicie okazało się, że jeśli wsadzę tego huba SignalR do konstruktora BattleManager, to dependency injection mi go dostarczy. Wtedy mogę sobie rozgłosić z tego huba co chcę, mimo że entry point to nie jest ten hub, tylko BattleManager.

Jednak! W ten sposób instancja tego huba trwa przez cały czas życia aplikacji na serwerze! Czy tak wolno? Co gorsza: Może się zdarzyć, ze dwie gry w tym samym czasie będą miały zakańczające się timery. Czy instancja hubu SignalR jest thread-safe? Bo jeśli nie, to muszę blokować go? Ale jeśli go zablokuję, to tym samym zaprzeczę sensowi używania ConcurrentDictionary i klasy BattleManager, która jest - ma być - thread safe. PRzecież ten framework ma być zaprojektowany, by działał na wielu wątkach.

Niedawno jednak pojawił się kolejny problem, na który znów nie miałem odpowiedzi. No bo chcę wreszcie zaimplementować to zapisywanie zakończonych gier do bazy danych, by móc obsługiwać replay'e. Zatem muszę mieć dostęp do Entity Framework Core. No ale Entity Framework jest Scoped: zakazane jest wywoływanie Scoped w Singletonach. Mogę co prawda uczynić hub SignalR zależnym od EF, wtedy dependency injection da mi EF(skąd, drogą eliminacji, musimy wnieść, że hub SignalR jest Transient?)... no ale znowu: z powodu timerów, MUSZĘ BYĆ W STANIE WYWOŁAĆ BAZĘ DANYCH Z BattleManagera! Czyli zrobić dokładnie to, czego framework mi zakazuje.

Kolejne pytanie na Stacku. Okazuje się że rzeczywiście, można jednak (technicznie) w Singletonie zażądać sobie jakiegoś Scoped Service. W związku z tym metoda odpowiadająca za zakańczanie rozgrywek w BattleManager wygląda teraz następująco:

        // The battle here must be locked by the caller
        public void removeBattle(ulong battleId)
        {
            // Only this place of code removes battles. So I think there may be no such leaks that some timers are left undisposed?
            var battleState = battles[battleId];
            battleState.disposeTimers();
            ((IDictionary<ulong, BattleState>)battles).Remove(battleId);
 
            var replay = new Replay(battleId, battleState, DateTime.Now.ToUniversalTime());
            var playerIds = battleState.players.Keys.Select(userInfo => userInfo.UserIdentifier);
            _ = saveReplayAsync(replay, playerIds);
        }
 
        public async Task saveReplayAsync(Replay replay, IEnumerable<string> playerIds)
        {
            using (var scope = scopeFactory.CreateScope())
            {
                var db = scope.ServiceProvider.GetRequiredService<ApplicationDbContext>();
                await db.replays.AddAsync(replay);
                foreach (var playerId in playerIds) await db.AddAsync(new ReplayPlayers(replay, playerId));
                await db.SaveChangesAsync();
            }
        }

Tak - metoda removeBattle zakłada, że odnośny BattleState jest zablokowany przez callera. Może to i jest spaghetti, ale naprawdę nie mam pomysłu na nic lepszego.

Powyższy kod wołany jest zarówno, gdy gra się skończy normalnie (któryś z graczy wykonuje wygrywający ruch), jak i wówczas, gdy timer się zakańcza.

Działa. Ale teraz to już jestem pewien, że odstawiam cuda na kiju i że framework absolutnie nie jest przewidziany do podobnych zabaw. Co gorsza, w powyższym pytaniu na Stacku dowiedziałem się jednak, że nie mogę mieć wielu Timerów aktywnych jednocześnie, choćby z powodu marnowania pamięci i procesora (i to, podobno, sporego marnowania)! Zamiast tego mam jednak zrobić "background task", odpalający się co np. 20 sek! Nie rozumiem tego. Przecież wtedy Timery będą się zakańczać z dokładnością do 20 sek. To znaczy, że gdy graczowi skończy się czas, to jego przeciwnik będzie musiał czekać do 20 sek., aby oficjalnie zostać zwycięzcą? No nie powinno tak to wyglądać.

Aha. Ludzie na Stacku sobie przeczą. No bo, o ile dobrze rozumiem, pod pierwszym pytaniem odradzano mi dokładnie to samo rozwiązanie, które pod drugim pytaniem mi doradzano? No cóż, można się było tego spodziewać. Ale w takim razie co ja mam zrobić, skoro mnie samemu brak wiedzy, by rozeznać?

Hmm... planuję kolejne posty kwestii tej gry.. aż przydałby sie tag... #mon

kmph

@Afish: A nie muszę robić czegoś na tej zasadzie? https://docs.microsoft.com/en-us/ef/core/saving/concurrency Tak jak rozumiem transakcje, to one gwarantują mi TYLKO, że albo wszystkie zmiany są zapisane, albo żadna. Ale przecież nie do końca przed tym chcę się bronić. Załóżmy np., że mamy turę, w której wymagane są komendy od obu graczy, by móc ruszyć dalej grę. Tak więc BattleState ma zaznaczone, iż oczekuje na komendy od obu. Teraz - co chyba wcale nie jest nierealne? - obaj niemal naraz wysyłają swoje komendy. Więc teraz tak: wątek 1 otwiera transakcję z komedną od gracza 1, wątek 2 otwiera transakcję z komendą od gracza 2, wątek 1 zapisuje komendę gracza 1, zaznaczjąc, że teraz czeka się na komendę od gracza 2 i zamyka transakcję, jako że wątek 2 otworzył transakcję, to operuje on wciąż na danych sprzed zapisu wątku 1, więc zapisuje komendę gracza 2, zaznaczając, że teraz czeka się na komendę od gracza 1 i... zamyka transakcję, nadpisując komendę od gracza 1, która jest stracona! Tak chyba jednak być nie może?

AD. EF będącym scoped: Sprawdzałem: Próba wsadzenia database context do konstruktora SingletonService rzuca wyjątkami. Oczywiście na upartego z pewnością da się to obejść i wsadzić jednak kontekst singletonowi, jednak dokumentacja przestrzega przed tym bardzo: https://docs.microsoft.com/en[...]injection?view=aspnetcore-2.1 Strasząc invalid states i takimi tam.

Jednak, moze jeśli mamy pozbyć się tego ConcurrentDictionary i oprzeć się tylko na bazie danych, to nie ma to znaczenia, bo już nie trzeba mieć żadnych Singletonów? A tak zamierzam zrobić, i to już, nie później. Gdyż poległem na blokadach. Chcę zaimplementować obserwowanie trwających rozgrywek oraz listowanie ich. W tym celu chciałem blokować BattleState za pomocą ReaderWriterLockSlim. I co? Gucio... gdyż z jednej strony muszę je Dispose'ować, a z drugiej strony, nie wolno mi tego zrobić, póki jakikolwiek wątek czeka na blokadę! Jak mam to zapewnić? O tym dowiedziałem się dopiero poniewczasie, muszę wyrzucić nieco kodu do śmieci.

Afish

Odnośnie transakcji - nie rozumiesz poziomów izolacji i literki I z ACID-a.
Odnośnie EF - nie znam DI z core'a, więc nie pomogę.
Wszystko, co chcesz zrobić na bazie, możesz równie dobrze zrobić na słowniku w pamięci, tylko musisz więcej kodu napisać.
Odnośnie blokad i Dispose - nie widziałem kodu, ale Twój opis sugeruje, że robisz coś bardzo źle.

kmph
2018-08-25 16:33

I dalszy ciąg problemików z moją gierką...

Tym razem problem krótki. Konwencje nazewnictwa.

Jakoś projekt nazwać musiałem, wymyśliłem więc roboczą nazwę mon. Zielonego pojęcia nie miałem, że Visual Studio stworzy mi namespace mon i jeszcze wetknie tam całkiem sporo kodu z szablonu.

No i teraz problem... No bo zorientowałem się, że konwencja jest, że namespace'y pisane są Wielkimi Literami. A więc: powinno być Mon, a nie mon. No więc, swój kod zacząłem wprowadzać w namespace Mon.

Jednak jest niekonsekwencja. Bo po pierwsze, jak już pisałem, Visual Studio już mi wsadził sporo kodu w namespace mon. Mało tego: sam jestem niekonsekwentny... bo jak tylko klikam, że chcę "new class", to automatycznie szablon wtyka mi ją w namespace mon. Jak zauważę, to poprawię na Mon, ale robi się to żmudne, a poza tym raz na jakiś czas coś przeoczę i parę fragmentów kodu wisi mi w mon. Ah, teraz zauważyłem... migracje itp. też są automatycznie umieszczane w mon!

Przede mną zatem przejrzenie i poprawienie zescaffoldowanego kodu... no i dokopanie się do settingu, żeby szablon przestał mi wiecznie umieszczać nowe pliki w namespace mon.

I kolejne problemy z konwencjami. No bo konwencja jest, że metody, properties i publiczne pola też są Wielkimi Literami. Ale ja jestem przyzwyczajony, że się je pisze małymi... Odruchowo piszę klasy i namespace'y Wielkimi, a wszystko inne małymi. W zasadzie wypadałoby to wszystko pozmieniać. No ale to żmudna robota, nie chce mi się, a poza tym nie mogę oprzeć się wrażeniu, że są ważniejsze rzeczy do poprawienia, aniżeli ta pierdópołka. Więc to za te ważniejsze się biorę.

Ale w takim razie ta dziwna konwencja pozostaje. No bo skoro już taka konwencja mi się zrobiła, no to dalej piszę według niej. Tym więcej zostawiam sobie do poprawiania na później.

I wreszcie bodaj najdziwniejszy problem:

public class SomeClass
{
    public SomeClass(int _someField, string _otherField)
    {
        someField = _someField;
        otherField = _otherField;
    }
 
    public int someField;
    public string otherField;
}

Tak wiem, te public biją po oczach, wiem że to wada, ale o tym już wcześniej pisałem. Teraz chodzi mi o co innego: o tę dziwną konwencję, że parametry konstruktora zaczynają się od _. Wzięło się to z tego, że nie wiedziałem, czy i jak w ciele konstruktora mogę rozróżnić między polem a parametrem o tej samej nazwie. Od początku przypuszczałem, że jest szansa, że zadziała coś takiego:

public class SomeClass
{
    public SomeClass(int someField, string otherField)
    {
        this.someField = someField;
        this.otherField = otherField;
    }
 
    public int someField;
    public string otherField;
}

Ale nie byłem pewny i nie sprawdziłem... gdyż byłem skoncentrowany na innym problemie... i już nie chciałem rozmieniać się na drobne. Więc zdecydowałem się na ten kludge.

Tak naprawdę dotąd nie wiem, czy powyższy kod (z tymi this) działa. Wiem natomiast, że jeśli już, to konwencja jest przeciwna: To pola (które nie maja być publiczne) mogą mieć _ przed nazwą, ale nie parametry konstruktora.

No i znowu - konwencja została, nie zmieniam wszystkiego no bo żmudne to (dowiedzenie się, jak powinno być, jest najłatwiejsze tutaj)... no i piszę dalej tak samo, no bo trzymam się tej już utartej konwencji.

Następny post będzie dłuższy... bo i problemik w nim opisany (dla mnie) trudniejszy.

#mon

furious programming

Zmienić go – nigdy nie jest za późno na refactoring. Obstawiam, że IDE którego używasz, posiada funkcjonalność wygodnej zmiany identyfikatorów (co aktualizuje wszystkie ich wystąpienia we wszystkich modułach), dzięki czemu nie będziesz musiał tego robić ręcznie.

Sam niedawno pozbyłem się prefiksów L dla zmiennych lokalnych w moim platformersie, który obecnie składa się z sześciu projektów, kilkudziesięciu modułów, łącznie z około czternastu tysięcy linii kodu – nie trwało to dłużej niż kwadrans. Gdybym miał ręcznie wszystko zmieniać, ciąg po ciągu, to siedziałbym przy tym dwa dni.

somekind

W ustawieniach projektu w Visual Studio można zmienić global namespace na prawidłową, wtedy nowo tworzone typy będą się w niej znajdowały. Co do starych namespaców, to albo ctrl + shif + f albo ReSharper -> Adjust namespaces, albo jakaś wtycznka, np.: https://marketplace.visualstu[...]itemName=p2410.NamespaceFixer

kmph
2018-08-13 20:04

Walki z projektem ciąg dalszy. Chyba nie zdążę przed końcem wakacji zrobić wszystkiego, co planuję :(

Mam dość poważne problemy z duplikacją kodu. Niestety, choć staram się to ograniczać (o ile tylko owo ograniczanie nie zatrzyma mnie zbyt mocno), copypasty jest tam nadal zdecydowanie za dużo. Nie wszędzie mam rozsądne pomysły, jak się tego pozbyć.

Weźmy na przykład public class Block : IAction. Mój oryginalny pomysł był taki, że utrzymywanie bloka (a) kosztuje niewielką ilość Staminy na turę, jeśli Staminy nie ma to blok jest zdejmowany; (b) natychmiast, gdy przeciwnik ukończy swój ruch, blok jest zdejmowany. Interfejs IAction wymaga metody execute - świetnie zatem, niech Block implementuje execute w ten sposób, że zabiera nieco Staminy, sprawdza, czy przeciwnik jest wolny i jeśli tak, zdejmuje się.

Działa. Ale! Oprócz Block mam jeszcze i Dodge, które działa bardzo podobnie, tyle że zamiast blokowania ataku przeciwnika (pewne zmniejszenie obrażeń o % wynikły ze stosunku Power walczących) usiłuje wykonać unik (przyjęcie albo 0% albo 100% obrażeń w zależności od RNG, szansa zależy od stosunku Speed obu walczących). No i obie klasy duplikują kod odpowiedzialny za zdejmowanie się wskutek braku Staminy albo wskutek faktu, że przeciwnik zakończył już swój ruch.

Co gorsza: Za chwilę okazuje się, że jednak Block oraz sposób jego nakładania i zdejmowania to są dwie różne rzeczy. No bo teraz np. chcę zrobić taki ruch jak Ram. Działa on tak: user podbiega do przeciwnika, blokując podczas podbiegania, następnie atakuje i zdejmuje bloka. No i mam problem. Jak to zrobić bez duplikacji kodu? Duplikowanie całej klasy Block oprócz kodu odpowiedzialnego za zdejmowanie to już przesada, nawet dla mnie.

Chcę wielodziedziczenia! Wtedy, myślę, dałbym sobie klasę Block, klasę Dodge, klasę RequireStaminaPerTurn, klasę UntilEnemyFinishesMove, klasę UntilEndOfChannel. I teraz niech NormalBlock dziedziczy po Block, RequreStaminaPerTurn, UntilEnemyFinishesMove. I niech NormalDodge dziedziczy po Dodge, RequireStaminaPerTurn, UntilEnemyFinishesMove. I wreszcie niech ChannelBlock dziedziczy po Block, RequireStaminaPerTurn, UntilEndOfChanel. Wszystko. Teraz nawet można (teoretycznie, nie zrobiłem tego) dać postać, która będzie miała taki passive, że będzie mieć bez przerwy założonego bloka bez żadnych kosztów Staminy.

Niestety! Wielodziedziczenie jest be i nie ma go w C#. Coś jednak muszę wymyślić! Z jednej strony nie chcę duplikować kodu ponad miarę, a z drugiej strony, nauczony tym doświadczeniem, chcę wprowadzić i utrzymać zasadę, że żaden efekt mający wpływ na grę nie odpowiada za swoje własne zdejmowanie i nakładanie.

W końcu wymyśliłem. Block nie implementuje już IAction. Zamiast tego pojawiły się takie klasy, jak: public class PlaceEffect: IAction, public class RemoveEffect : IAction, taki interfejs: public interface RequireSetup : IEffect { List<IAction> onPlacement { get; } List<IAction> onRemoval { get; } List<IAction> onFailure { get; } } itepe, itede.

Biorąc pod uwagę fakt, że dodatkowo jeszcze, celem umożliwienia, by tury wykonywały się jednocześnie (w sensie, obaj gracze dają polecenia i wtedy wykonuje się tura, nie ma znaczenia który gracz wykonuje swą turę jako pierwszy) wprowadziłem wymóg, by każda IAction implementowała property o nazwie priority - no zrobiło mi się spaghetti. Te wszystkie priority, getPriority, PlaceEffect, onBlahblah => { new List<IAction> { new PlaceEffect(_priority: yaddayadda, _effect: new JakiśtamEfekt(_onFailure : new List<IAction> {new RemoveEffect(plepleple.... nie chce mi się kończyć otwartych nawiasów i tym podobne... Wydaje mi się że brzydkie, obawiam się że błędogenne, i co gorsza podobne ruchy powtarzają ten niemiły kod czyli i tak mamy copypasty.

No ale idziemy dalej. Teraz implementuję sobie damage over time. Na pierwszy ogień idzie Hurricane i Conflagration. Te efekty odnoszą się do całego Field, toteż atakują obu graczy. No i tu już mamy problem, gdyż to ma działać jako efekt "stackowalny", tj: wykonanie ruchu Hurricane nakłada stack efektu Hurricane który to stack zostanie zdjęty po czasie zależnym od statystyk gracza nakładającego ten stack. Tak samo Conflagration. Co więcej, Hurricane i Conflagration razem tworzą kombo FieryTornado. I wreszcie, Hurricane, oprócz damage over time, nakłada jeszcze na obu graczy taki debuff, że każdy ich ruch kosztuje nieco więcej Staminy.

Zaimplementowane. Mam klasę Weather : IExecutable<Field>, IOnTire. Implementowanie IExecutable<Field> jest po to, by mogła nakładać obrażenia każdej tury. Implementowanie IOnTire jest po to, by mogła zwiększać zużycie Staminy. W niej klasy: PlaceWeatherStack : PlaceEffect, RemoveWeatherStack : RemoveEffect (ich zadanie: nakładanie / zdejmowanie instancji klasy Weather do zbioru efektów na Field kiedy tylko zostanie nałożony jakiś stack Hurricane lub Conflagration / kiedy ostatni stack zostanie zdjęty); no i oczywiście klasy HurricaneStack, ConflagrationStack. Klasa Weather, wywołana, sumuje sobie wszystkie stacki i na tej podstawie zadaje obrażenia / zwiększa użycie Staminy. Powinno działać. (Jeszcze nie przetestowałem).

Niestety. Klasa Weather, ze wszystkimi klasami zagnieżdżonymi, to jest sporo kodu. Więcej, niżbym przewidywał. 200 linii z hakiem.

A teraz nadchodzą kolejne dwa efekty: ToxicSpores i BurningSpores. Także zadają damage over time, ale już tylko na przeciwników. Nie mają także dodatkowych efektów takich jak IOnTire. Mimo to... duplikacja kodu pomiędzy tymi klasami a Weather jest straszna. Nieakceptowalna nawet dla mnie.

Ale przecież nie chcę! zrobić klasy bazowej, choćby abstrakcyjnej, która by zawierała w sobie zduplikowany kod! Przecież nie ma wielodziedziczenia, a pojedyncza klasa bazowa to przepis na powrót problemów, na jakie natknąłem się przy Block. W końcu (chyba) udało mi się wydzielić część zarządzania stackami:

public class PlaceStackedEffect : PlaceEffect
    {
        public interface IStackedEffect : IEffect
        {
            List<IStackManager> managers { get; }
 
            void prepare(Species affected);
 
            IStackedEffect accumulate(IStackedEffect eff, IStackedEffect acc);
        }
 
        public interface IStackManager : IEffect
        {
            void update(IEffectStorage storage);
        }
 
        public IStackedEffect eff;
 
        public PlaceStackedEffect(IStackedEffect _eff, List<IEffect> _others, List<int> _priority = null, IEffectStorage _target = null) : base(_others.Prepend(_eff), _priority, _target)
        {
            eff = _eff;
        }
 
        public override void execute(Species affected)
        {
            foreach (IStackManager required in eff.managers)
                if (!target.ownEffects.OfType<IStackManager>().Any(eff => eff.GetType().IsAssignableFrom(required.GetType())))
                    target.ownEffects.Add(required);
 
            eff.prepare(affected);
            base.execute(affected);
 
            foreach (IStackManager manager in target.ownEffects.OfType<IStackManager>().Where(sm => eff.managers.Any(required => sm.GetType().IsAssignableFrom(required.GetType()))))
                manager.update(affected);
        }
 
        public class RemoveStackedEffect : RemoveEffect
        {
            IStackedEffect eff;
 
            public RemoveStackedEffect(IStackedEffect _eff, List<IAction> _onRemove = null, List<int> _priority = null, IEffectStorage _target = null) : base(_eff, _onRemove, _priority, _target)
            {
                eff = _eff;
            }
 
            public override void execute(IEffectStorage affected)
            {
                base.execute(affected);
 
                foreach (IStackManager required in eff.managers)
                    foreach (IStackManager manager in target.ownEffects.Where(eff => eff.GetType().IsAssignableFrom(required.GetType())))
                        manager.update(affected);
            }
        }
    }

Wciąż nie usuwa to znacznej części duplikacji... np. sumowanie damage jest analogiczne we wszystkich 3 przypadkach, ale tego nie chcę wydzielać do IStackedEffect, bo łatwo wyobrazić sobie taki efekt, który by się zupełnie inaczej agregował. Tak samo analogiczny jest kod odpowiadający za dispelowanie stacków, ale znowu: boję się powrotu problemów spod znaku Block (tj. duplikacja kodu wymuszona brakiem wielodziedziczenia). No bo co, jeśli jakiś inny efekt stackowalny będzie inaczej dispelował stacki i co więcej, tak jak w przypadku Block / Dodge, jego inne dispelowanie też winno być dziedziczone po iluśtam innych efektach?

Przyznaję, że ten projekcik jest dla mnie solidną lekcją pokory.

#mon

Afish

Jak się zastanawiasz, czy przepisywać teraz, to nie przepisuj, dzięki temu kiedyś dojdziesz do momentu, w którym będziesz wiedział, kiedy to potencjalnie należało przepisać i czy ten moment przegapiłeś, a to też jest cenna wiedza.

wnuq

Nie ma sensu przepisywać teraz, tak jak to napisał i wyjaśnił przedmówca - do tego plus dla Ciebie, że zdałeś sobie sprawę z tego, że Twój obecny kod "śmierdzi" na kilometr. Natomiast przy kolejnym projekcie, koniecznie pisz automatyczne testy. Wtedy nie tylko łatwiej będzie coś zmienić ale i trudniej będzie bezsensowny kod napisać. Zapoznaj też się z wzorcami projektowymi, pomoże uniknąć Ci to duplikacji kodu, a gdy już coś pójdzie nie tak to z technikami refaktoryzacji. Zaś jeśli jesteś ambitny to poleciłbym też książki takie jak Clean Code i Clean Architecture.

kmph
2018-07-28 17:20

Wyznaję, że zaczynam rozumieć sens podziału klas na namespace'y.

Jak głupi wsadziłem całą mechanikę rozgrywki do namespace Mechanics - w odróżnieniu od namespace Hubs (sieć) czy namespace Chat. I tak tenże namespace zawiera sobie plik Moves.cs, Actions.cs (klocki, z których budujemy Moves), Species.cs, Effects.cs itd.

W konsekwencji, na chwilę obecną, taki np. Moves.cs zawiera 1740 linii i mnóstwo małych klas, włącznie z klasą abstrakcyjną Move. Actions.cs natomiast zawiera klasę abstrakcyjną Action i klasy dziedziczące po niej, razem 866 linii.

Moje rozumowanie było takie, wszystkie te klasy są już tym połączone, że dziedziczą po tej samej klasie abstrakcyjnej, dawanie jeszcze osobnego namespace'u na to (przy istnieniu i tak klasy abstrakcyjnej) to by było masło maślane.

No ale teraz mam problem: Mam sobie public class Block : Move, który używa... public class Block : Action. Ugh. Nie można tak. Chciałoby się dać: public class BlockAction : Action, no ale to jest przecież marna imitacja namespace'ów!

No więc znalazłem sobie synonim: public class Mitigate : Action. Ale! Nie tylko Block korzysta z tej Action! Mamy przecież jeszcze ruchy analogiczne, różniące się szczegółami: Fortify, Parry, itd... Zaczyna mi brakować synonimów na nazewnictwo!

A problemów tego rodzaju zaczyna być coraz więcej. Do tego stopnia coraz więcej, że w końcu się ugiąłem i zacząłem walić nazwy takie, jak FrenzyEffect (bo koliduje z ruchem Frenzy, który kładzie ten Effect).

A przecież rozwiązanie byłoby proste: public namespace Moves, public namespace Actions, public namespace Effects, itd. Zamierzam tego dokonać, ale później: teraz zbyt się spieszę, żeby w ogóle zdążyć z projektem przed końcem wakacji... (Gdyż wbrew pozorom ten refactoring nie będzie trywialny, gdyż z powodu trzymania zbyt wielu rzeczy w jednym pliku zacząłem traktować niektóre klasy jakby były namespace'ami i w efekcie mamy public class Ablaze : Effect zagnieżdżoną w public class Fire : Type. Aby zrobić ten porządek, trzeba będzie powyciągać takie kwiatki i porozdzielać po różnych folderach.)

Problem z nazewnictwem, który się zazębia: dziwactwa języka angielskiego. W tej konkretnej kwestii namespaceów jest to problem teoretyczny: np. co gdybym miał klasę abstrakcyjną Sheep i dziedziczące po niej różne rasy owiec? Jak wtedy nazwać namespace? Sheep? Pójdzie, ale zdaje mi się, że lecą warningi kompilatora wtedy. To co, Sheeps? Ugh. Wali po oczach równo, ale lepszego wyjścia nie widzę.

Zresztą na podobny problem się już natknąłem w praktyce. Otóż mam sobie public class Attack : Action. No i mam także efekty dziedziczące po interfejsach IOnAttacker, IOnAttack, IOnAttacked. Jak na razie pasuje. Ale! Mam także public class Hit : Action. I podobnie, mam interfejsy IOnHitter, IOnHit, IOnHit.... No właśnie. Past Participle dla Hit jest po angielsku identyczna z Infinitive, która z kolei jest identyczna z rzeczownikiem.

Musiałem więc być kreatywny: IOnHitter, IOnHit, IOnHitted.

Wali po oczach.

Ale trudno.

#mon

kmph

@kamillapinski: ? Jeśli piszemy Actions.BlockAction to czemu w ogóle nie zarzucić namespace'ów i nie pisać po prostu BlockAction? Z drugiej strony, jeśli to rzeczywiście takie ważne, to czemu po prostu nie importować w ciemno i nie pisać za każdym razem Actions.Block?

kamillapinski

Bo Block to nie to samo co BlockAction. Wyobraź sobie, że masz otwarte w kartach 2 pliki zawierające klasy w dwóch przestrzeniach nazw, ale nazywające się tak samo. Np. Actions.Block i Objects.Block. Gwarantuję, że prędzej czy później gdzieś się pomylisz i zaczniesz edytować zły plik. Namespaców nie warto zarzucać, bo: 1. pozwalają na logiczne grupowanie klas, 2. pozwalają na uniknięcie wieloznaczności przy korzystaniu z bibliotek (np. System.Collections.Generic.List vs My.Collections.List). Punkt 1 działa tylko wtedy, jeśli pliki w jednym namespace są w tym samym folderze.

kmph
2018-07-20 03:15

Walki z C# ciąg dalszy.

Solo projekt z właśnie minionego semestru na uni - grę napisać. Oddana, zaliczona. Ale ja się uparłem, by do końca wakacji ją na tyle podciągnąć, by móc ją chociaż jako wersję alpha upublicznić.

K...a... ja nie wiem, może powinienem się cieszyć, bo C# blokuje mi różne antipatterny, w które bym popadł... Ale ciągle mam poczucie, że... walczę z C#, nie tylko z własnymi brakami w doświadczeniu. Co chwila chcę zrobić coś, co mi się wydaje oczywiste i... język nie wspiera tej konstrukcji.

Np:

public abstract class WeatherEffect : IEffect
{
    public long power;
 
    public WeatherEffect(long _power)
    {
        power = _power;
    }
}
 
public class PlaceWeatherEffect<Eff> : PlaceEffect where Eff : WeatherEffect
{
    public PlaceWeatherEffect(long _power)
        : base(new List<IEffect> { Eff(_power) }, _priority: /*blah blah*/)
        {}
}

Tak, wiem, ten public long power pewnie bije po oczach :( Rzeczywiście, jakoś najłatwiej mi iść z polami publicznymi :( wielokrotnie dawałem już pola prywatne, a 3 dni później miałem "o cholera, jakaś inna klasa koniecznie potrzebuje dostępu do tego pola" no i robiłem je publiczne, aż w końcu zacząłem wszystko z defaultu robić publiczne. Pewnie nie jest to najlepsze, ale w kilku miejscach jedyne sensowne wyjście, jakie widzę, oprócz tego, to dać publiczne gettery i settery, a to przecież na jedno wychodzi.

Istotniejsza kwestia jest, że ten kod nie działa. Przecież konstruktory nie są dziedziczone. Ba! nawet dodanie interfejsu nie pomoże, bo nie można w interfejsie wymagać konstruktorów!

Grr. Wrr. No cóż, spróbujmy inaczej:

public abstract class WeatherEffect : IEffect
{
    public long power;
 
    public WeatherEffect() {}
 
    public WeatherEffect(long _power)
    {
        power = _power;
    }
 
    public void ConstructorThatCannotBeMadeConstructor(long _power)
    {
        power = _power;
    }
}
 
public class PlaceWeatherEffect<Eff> : PlaceEffect where Eff : WeatherEffect, new()
{
    public PlaceWeatherEffect(long _power)
    {
        Eff eff = new Eff();
        eff.ConstructorThatCannotBeMadeConstructor(_power);
        base(new List<IEffect> { eff }, _priority: /*blah blah*/);
    }
}

No i w ten sposób udało się dodać niepotrzebne niepoprawne stany do klasy WeatherEffect. Źle?

Ale co istotniejsze... ten kod nadal nie działa. Nie wolno wołać base w ciele konstruktora.

public class PlaceWeatherEffect<Eff> : PlaceEffect where Eff : WeatherEffect, new()
{
    private Eff getEffect(long _power)
    {
        Eff eff = new Eff();
        eff.ConstructorThatCannotBeMadeConstructor(_power);
        return eff;
    }
 
    public PlaceWeatherEffect(long _power) : base(new List<IEffect> { getEffect(_power) }, _priority: /*blah blah*/) {}
}

Co, znowu źle? No nie, zaczynam być naprawdę zły.

public class PlaceWeatherEffect<Eff> : PlaceEffect where Eff : WeatherEffect, new()
{
    public PlaceWeatherEffect(long _power) : base(new List<IEffect> {}, _priority: /*blah blah*/)
    {
        Eff eff = new Eff();
        eff.ConstructorThatCannotBeMadeConstructor(_power);
        effectsToPlace.Add(eff);
    }
}

I tym sposobem pole effectsToPlace w klase PlaceEffect stało się protected. Było private.


Przymuję do wiadomości, że problem może leżeć po mojej stronie. Być może chodzi o to, że C# to "czysty" język, więc "by design" zabrania rozmaitych antipatternów, w które niedoświadczeni (jak ja) mogliby wpadać. Być może prawidłowe rozwiązanie jest zupełnie inne (jakie?) Niemniej:

  • Rozwiązanie, do którego w końcu doszedłem, wydaje się być najbrzydsze z możliwych; na pewno bardziej "anitpatternowe", niż to, co chciałem na początku;
  • Facet na uni reklamował nam C# jako język bardzo intuicyjny, taki, że ludzie nie mający żadnej styczności z programowaniem niemal od razu coś sobie piszą. Uff... Więc chyba moja intuicja rozmija się z intuicją innych.

#mon

kamillapinski

List scastuje się w górę, o to w tym chodzi :). Dobra zasada jest taka, żeby zawsze jako argument przyjmować interfejsy, względnie klasy abstrakcyjne. Jeśli potrzebujesz funkcjonalności listy, to jako parametr dajesz IList, jeśli chcesz tylko zrobić for dajesz IEnumerable, jak chcesz sprawdzać, czy element istnieje i ewentualnie dodawać nowe, używasz ICollection itd. Chodzi o to, żeby nie ograniczać się do konkretnej implementacji. Chcesz powiedzieć kompilatorowi, jakiej funkcjonalności wymagasz, a nie jakiej klasy.

somekind

Jeśli czegoś się nie da zrobić konstruktorem, to się nie robi w ten sposób i tyle. Nie trzeba cudować.