Jak mądrze usunąć powtarzalność kodu w tej funkcji?

private void AdvanceTurn()
{
    AddNewUpdateChunk();
    foreach (var team in Teams)
        if(team.Active.Move != null)
            team.Active.Move.Init(team.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        if(team.Active.Move != null)
            team.Active.Move.OnTimeStep(team.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        if(team.Active.Move != null)
            team.Active.Move.OnAttack(team.Active, team.OtherTeam.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        TransitionResources.Apply(team.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        CheckFainted.Apply(team.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        if (team.Active.Move != null && team.Active.Move.IsCompleted)
            team.Active.Move.Remove(team);

    AddNewUpdateChunk();
    checkForceTie.Apply(this, updates);
}

Powtarzalność kodu jest widoczna na pierwszy rzut oka. Nie za bardzo mam pomysły, co z tym mądrego zrobić. Zdaję sobie sprawę, że idealnie winno to pewnie wyglądać tak:

private void AdvanceTurn()
{
    InitMoves();
    MovesOnTimestep();
    MovesOnAttack();
    ApplyTransitionResources();
    ApplyCheckFainted();
    RemoveCompletedMoves();
    ApplyForceTieChecker();
}

Ale to tylko zepchnie problem, bo np. metoda MovesOnTimestep będzie wyglądać tak:

private void MovesOnTimestep()
{
    AddNewUpdateChunk();
    foreach (var team in Teams)
        if(team.Active.Move != null)
            team.Active.Move.OnTimeStep(team.Active);
}

A metoda MovesOnAttack będzie wyglądać dokładnie analogicznie, więc powtarzalność kodu pozostanie.

Problemów z powtarzalnością możnaby tu wymienić kilka:

Po pierwsze: bez przerwy wołane jest AddNewUpdateChunk na początku. Ale ponieważ AddNewUpdateChunk JUŻ jest pojedynczą metodą, to jedyny sposób na usunięcie powtarzalności, jaki widzę, to coś takiego:

private delegate void Del();

private void AdvanceTurn()
{
    var l = new List<Del>{
        InitMoves, MovesOnTimestep, MovesOnAttack,
        ApplyTransitionResources, ApplyCheckFainted,
        RemoveCompletedMoves, ApplyForceTieChecker
    };

    foreach(var m in l) {
        AddNewUpdateChunk();
        this.m();
    }
}

Na razie wygląda pięknie - ale gdyby tylko do środka tej listy powędrowało wywołanie jakiejś nowej metody, która już nie chce być poprzedzona AddNewUpdateChunk();, to natychmiast okazałoby się, że robiąc powyższe strzeliłem sobie w stopę.

Drugim powtarzalnym elementem są te iteracje po teamach i active. Tu już niemalże w ogóle nie widzę, co zrobić - przecież metoda musi jakoś zaznaczyć, czy iteruje, czy nie. Tzn jedyna mądra rzecz, jaką widzę, to zamknąć iterację po aktywnych stworkach w każdej drużynie w jednej linijce:

private IEnumerable<(Team, Species)> MonsWithNotNulledMoves => Teams.
    Zip(Teams.Select(t => t.Active), (t, m) => (t, m)).
    Where((t, m) => m.Move != null).
    Select((t, m) => (t, m, m.Move)); // czy jakos tak

private void AdvanceTurn()
{
    AddNewUpdateChunk();
    foreach (var (_, mon, move) in MonsWithNotNulledMoves)
        move.Init(mon);

    AddNewUpdateChunk();
    foreach (var (_, mon, move) in MonsWithNotNulledMoves)
        move.OnTimeStep(mon);

    AddNewUpdateChunk();
    foreach (var (team, mon, move) in MonsWithNotNulledMoves)
        move.OnAttack(mon, team.OtherTeam.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        TransitionResources.Apply(team.Active);

    AddNewUpdateChunk();
    foreach (var team in Teams)
        CheckFainted.Apply(team.Active);

    AddNewUpdateChunk();
    foreach (var (team, _, move) in MonsWithNotNulledMoves)
        if (move.IsCompleted)
            move.Remove(team);

    AddNewUpdateChunk();
    checkForceTie.Apply(this, updates);
}

No ale to jest tylko połowiczne rozwiązanie powtarzalności, większość powtarzalności zachodzi dalej...

Nie jest to szczyt efektywności, ale

var goodTeams = teams.Where(x => x.Active.Move != null).ToList();

a w tym jednym przypadku

foreach (var team in goodTeams.Where(x => x.Active.Move.IsCompleted))

?

Anyway wygląda mi to na tego typu przypadek:

I’ve usually heard this phenomenon called “incidental duplication,” and it’s something I find myself teaching junior engineers about quite often.

There are a lot of situations where 3-5 lines of many methods follow basically the same pattern, and it can be aggravating to look at. “Don’t repeat yourself!” Right?

So you try to extract that boilerplate into a method, and it’s fine until the very next change. Then you need to start passing options and configuration into your helper method... and before long your helper method is extremely difficult to reason about, because it’s actually handling a dozen cases that are superficially similar but full of important differences in the details.

Może jednak lepiej zostawić ten kod tak, bo jest perfectly readable, niż wymuszać takie rzeczy foreach (var (team, mon, move) in MonsWithNotNulledMoves

lub listy actionów (właściwie czemu tam delegate dałeś?)

Ugh... Przeczytałem Refactoring 101 https://over... Twój wpis na blogu i arty, które tam były zalinkowane...

A także ten art: https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction (link od Goodbye Clean Code)

Już teraz mam wątpliwości co do innych fragmentów kodu.

Channel na przykład: wyświetla defaultową wiadomość i wyświetla ikonkę. Przypuszczam, że w większości wypadków to jest OK. Ale są wypadki gdzie trzeba wyświetlić inną ikonkę albo inny tekst walnąć... Inny przykład, obliczanie damage'a, w środku są znowu wywalane status message'y na konsolę ale choć do tego punktu jeszcze nie dotarłem już przypuszczam, że będę musiał w niektórych wypadkach wyłączyć te wiadomości na konsolę...

Czy to znaczy że mam zinline'ować to wszystko?

Czy jednak czasami jest sens przyjąć jakieś defaulty (jeśli w większości wypadków są one sensowne) i wyłączyć je parametrami, gdy akurat nie są sensowne?

Zinline'owanie tego wszystkiego, co obecnie ma jednak w środku parametry / conditionale / lub silnie przypuszczam że szybko mieć je zacznie już obawiam się że doprowadziłoby duplikację naprawdę do przesady...

Hej Twoim problem jest to że operujesz na złych poziomach abstrakcji, tzn na jakiś listach tablicach czy czymś analogicznie nisko poziomowym. Zamiast iterować poniej powinieneś przerobić Teams na osobną wyspecjalizowaną klasę, lub napisać klasę do wołania akcji. Do którejś wersji \ .Net'a implementacja kolekcji była dość toporna, (bo interfejsy np. IList<T> są spore)i każdy nauczył się unikać, dlatego nie ma za wiele przykładów w literaturze ale implementacja IReadOnlyCollection<T> IReadOnlyList<T> jest trywialna. IReadOnly implementują wszystkie standardowe kolekcje.

Wracając do rzeczy po przeniesieniu iteracji poziom wyżej otrzymasz coś w stylu:

Teams.MoveForActive(active=> active.Move.Init(active));
Teams.MoveForActive(active=> active.Move.OnTimeStep(active));
...
Teams.ForActive( CheckFainted.Apply);
Teams.ForActive( TransitionResources.Apply)
...

Co wygląda lepiej, ostanie linie, wyglądają prawie jak język angielski. Pierwsze tez można tak przerobić. Tu sztuczka polega na tym żeby że przekazujemy selektor metod które chcemy użyć, a wolamy je z dobrym paramtrem pod spodem. Wyjdzie takie coś:

Teams.MoveCallOnActive(move => move.Init)
Teams.MoveCallOnActive(move=>move.OnTimeStep);
...
Teams.ForActive( CheckFainted.Apply);
Teams.ForActive( TransitionResources.Apply)
```

To czy piszemy Teams jako osobną kolekcje lub czy je w rapujemy klasą, zależy od tego, czy chcemy usnąć ` AddNewUpdateChunk();` i na jakim poziomie on się znajduje. Jeśli ta metoda jest wywoływana tylko w tym kontekście, to lepiej napisać wraper. Stworzyć instancje kiedy na początku metody, wykonać nasze iteracje i zapomnieć o nim.

Ed. Moje rozwiązanie nie łapie sie pod te artykuły, bo nie tworze żadnych dziwnych metod helperow, ktore trzeba utrzymywać parametryzowac itd. i nie robie tez niczego czego nie mozna zrobić na gołej liście, w kilku krokach. Wystawiam tylko najczescie występujące przypadki jako metody, coś jak `max`, `min`, `where` itd.

Wczoraj nie miałem dostępu do IDE, implementacja.

        void ForEachActive(Action<IActive> callback)
        {
            foreach (var team in _teams)
            {
                callback(team.Active);
            }
        }

        void MoveEachActive(Func<IMove,Action<IActive>> selector)
        {
            foreach (var team in _teams)
            {
                var move = team.Active.Move;
                if (move != null)
                {
                    selector(move).Invoke(team.Active);
                }
            }
        }