Как сделать из этого метода нормальный код?

Question

[Дядя Богдан]

Как из этого сделать нормальный, красивый, читаемый и вообще самый лучший код?

PS Добавил C# в тег, так как особые знания Unity тут не нужны

public List<Cell> CalculateVariants(List<Vector2> movevariants, List<Vector2> attackvariants)
    {
        var variants = new List<Cell>();

        foreach (Vector2 offset in movevariants)
        {
            if (GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit == null) != null)
            {
                variants.Add(GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit == null));
            }
        }

        foreach (Vector2 offset in attackvariants)
        {
            if (GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit != null) != null)
            {
                variants.Add(GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit == null));
            }
        }

        return variants;
    }

Comments

[Василий Банников]

Что такое GridRepository.Cell?

[Дядя Богдан]

Василий Банников, Все части сетки.

Answer 1

[GavriKos]

Избавиться от дубликатов в виде Find, как минимум. Как можно больше кешировать.

Comments

[Дядя Богдан]

А как от них избавится то?

Только так получилось:

public List<Cell> CalculateVariants(List<Vector2> movevariants, List<Vector2> attackvariants)
    {
        var variants = new List<Cell>();
        var cell = new Cell();

        foreach (Vector2 offset in movevariants)
        {
            cell = GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit == null);

            if (cell != null)
            {
                variants.Add(cell);
            }
        }

        foreach (Vector2 offset in attackvariants)
        {
            cell = GridRepository.Cells.Find(cell => cell.Position == cell.Position + offset && cell.Unit != null);

            if (cell != null)
            {
                variants.Add(cell);
            }
        }

        return variants;
    }

[GavriKos]

Дядя Богдан, ну уже неплохо

cell.Position == cell.Position + offset

А вот это разве логично? Ведь достаточно тогда просто offset на ноль проверить. Очень странное условие крч

[Дядя Богдан]

GavriKos, Ну типо он проверяет, если есть клетка с позицией которая например на одну клетку дальше игрока( X = 0, Y = 1), тогда заносить ее в лист вариантов хода, типо проверка есть ли клетка для хода.

[GavriKos]

Дядя Богдан, вы же понимаете что эта проверка истина только тогда, когда offset равен нулю? Либо есть логическая ошибка

[Дядя Богдан]

GavriKos, ХЗ. В прошлой моей игре была такая проверка и все работало.

Проверка из прошлой игры:

public Cell FindCellByPositon(Vector2 Offset, Cell Cell)
    {
        return AllCell.Find(cell => cell.Position == Cell.Position + Offset && cell.UnitInCell == null);
    }

[Foggy Finder]

Дядя Богдан, она у вас не такая, в прошлой проверке вы сравниваете значения каждой ячейки (cell) с конкретной ячейкой (Cell)

[Дядя Богдан]

Foggy Finder, Тупанул

[Lesh48]

Конечно же заменить foreach на for (т.к. foreach не так оптимизирован (хотя компилятор должен сам его заменить на обычный цикл)) и не использовать var, где заранее известен тип.

Answer 2

[Василий Банников]

Код в целом норм, если не упарываться в Linq.
Использовать linq не советую, тк он заметно медленнее и потребляет больше памяти, чем циклы.

Если рефакторить, то целиком, включая GridRepository.
Например можно зная текущую клетку и оффсет найти за O(1) все нужные клетки и отбросить занятые.

Comments

[Дядя Богдан]

GavriKos написал что можно избавиться от дубликатов Find. Такое возможно без черной магии Linq?

[Иван Спасов]

Дядя Богдан, можно сохранить результат find в переменную

[Василий Банников]

Иван Спасов, а, да. Это не заметил.

Answer 3

[Developer]

LINQ используй

Comments

[Дядя Богдан]

Так че там нормального. Как он поможет сократить код?