@nosochek
самоучка, шакал(иногда картошечка)

Правильно ли такое структурирование кода в контексте ООП?

Вопрос к разбирающимся в ООП людям. Я самостоятельно вкатываюсь в ООП (читаю книжку "Принципы, паттерны и методики гибкой разработки на языке C#" и перерабатываю свои старые простенькие консольные программы, пытаясь привести их к тому, что я понял как ООП). В силу того что в окружении я не имею экспертов в программировании (не говоря уже о ООП), то мне банально не у кого спросить правильно ли я делаю и что нужно (если нужно) исправить. Так вот решил я свои труды вывалить на всеобщее обозрение, надеясь получить критику или советы.

Сначала в общем, есть у меня приложение, которое симулирует поведение песка, переписывать его я решил с составления UML диаграммы (которую я, правда, закончил только вместе с самим кодом). Диаграмма выглядит так:
6148b29b2d76e166640408.png

так же код всех элементов:
interface IFallable
    {
        public void Fall();

        public bool CanFall(ParticleStorage ps);

    }


abstract class Particle
    {
        public int x;
        public int y;
        public char c = '%';
        public Particle(int _x, int _y)
        {
            x = _x;
            y = _y;
        }
        public virtual (int, int) GetPosition()
        {
            (int, int) Position = (x, y);
            return Position;
        }

        public virtual char GetChar()
        {
            return c;
        }
    }


class Sand : Particle, IFallable
    {
        public Sand(int x, int y) : base(x, y)
        {
            c = '0';
        }

        public void Fall()
        {
                y += 1;
        }

        public bool CanFall(ParticleStorage ps)
        {
            if (ps.GetParticleByPosition(x, y + 1) == null)
            {
                return true;
            }
            else
            {
                return false;
            }
        }

    }


class Block : Particle
    {
        public Block(int x, int y) : base(x, y)
        {
            c = '#';
        }

    }


class ParticleStorage
    {
        List<Particle> particles;
        public ParticleStorage(List<Particle> p)
        {
            particles = p;
        }
        public List<Particle> GetParticles()
        {
            return particles;
        }

        public Particle GetParticleByPosition(int x, int y)
        {
            Particle returnedParticle = null;
            foreach (Particle p in particles)
            {
                if(p.x  == x && p.y == y)
                {
                    returnedParticle = p;
                }
            }
            return returnedParticle;
        }
    }


class Mover
    {
        public void MoveAllParticles(ParticleStorage ps)
        {
            List<Particle> particles = ps.GetParticles();
            foreach (Particle p in particles)
            {
                if(p is IFallable)
                {
                    IFallable f = (IFallable)p;
                    if (f.CanFall(ps))
                    {
                        f.Fall();
                    }
                }
            }
        }
    }


public void DrawAllParticles(ParticleStorage ps)
        {
            List<Particle> particles = ps.GetParticles();
            foreach (Particle p in particles)
            {
                Console.SetCursorPosition(p.GetPosition().Item1, p.GetPosition().Item2);
                Console.Write(p.GetChar());
            }
        }
    }


class Program
{
    static void Main()
    {
        Console.CursorVisible = false;
        Drawer d = new();
        Mover m = new();
        Sand s = new(3, 3);
        Block w = new(3, 6);

        List<Particle> particles = new List<Particle>();
        particles.Add(s);
        particles.Add(w);

        ParticleStorage ps = new ParticleStorage(particles);

        while (true)
        {
            d.DrawAllParticles(ps);
            m.MoveAllParticles(ps);
        }
    }
}
  • Вопрос задан
  • 152 просмотра
Решения вопроса 1
FoggyFinder
@FoggyFinder
Для начала сделаю оговорку что основным языком программирования для меня является F#, который в первую очередь функциональный, но пару общих советов, рекомендации и просто мыслей, в том числе и по ООП, оставлю.

1. Класс Particle. Запомните - никаких публичных полей в классах быть не должно. Это грубое нарушение инкапсуляции. Соответственно метод GetChar не имеет смысла ведь доступ к информации и так есть.

Что касается метода GetPosition то существующие наследники класса не переопределяют его а значит нет необходимости делать его виртуальным. Кроме того все его использования легко заменить на обращения к свойствам и создавать для таких целей кортеж, на мой взгляд, лишнее.

abstract class Particle
{
    public int X { get; protected set; }
    public int Y { get; protected set; }
    public char C { get; protected set; } = '%';

    public Particle(int _x, int _y)
    {
        X = _x;
        Y = _y;
    }
}


2. Интерфейс IFallable. Не понятен заложенный в нем смысл. Что он должен отражать? Некоторый объект который может падать? Если да, то

а) подобный интерфейс был бы уместен в том случае если определенное поведение присуще разным, несвязанным между собой типам.
б) строгая зависимость от ParticleStorage. [Edit: Здесь имеется ввиду что в контракте CanFall параметром идет ParticleStorage] Интерфейс становится совершенно не гибким.

Если вашим намерением было отделить тип частица от типа частица которая способна падать то, возможно, лучшим решением было бы создать новый абстрактный класс наследник от Particle?

abstract class FallableParticle : Particle
{
    protected FallableParticle(int _x, int _y) : base(_x, _y)
    {
    }

    public abstract void Fall();
}


---
[Edit: На вопрос что лучше использовать абстрактный класс или интерфейс во многих ситуациях нет однозначно правильного ответа. Все зависит от будущего использования. В C# нет множественного наследования (что прекрасно) и если есть необходимость в описании разных поведений (падание, горение) то, конечно, применение интерфейсов кажется более подходящим.

Так что тут все зависит от контекста и планируемого использования, расширения. Другими словами - если другой функционал (горение) в ближайшем будущем не предполагается реализовывать то оставить абстрактный класс кажется адекватным решением. Если планируется, пусть и на перспективу, лучше использовать интерфейсы.]
---

Обратите внимание: я полностью убрал метод CanFall - он не нужен. Падающая частица может падать по своему определению. Если ее способность перемещаться определяется извне то и проверка должна быть там.

[Edit: В данном случае проверка перемещения в классе Mover

if (f.CanFall(ps))
{
    f.Fall();
}


это хорошее решение. Mover это конкретная реализация какого-то абстрактного перемещения в абстрактном пространстве. Тут все нормально и дополнительная сущность была бы чрезмерным усложнением.

Сами проверки могут быть многоуровневыми. Например, мы могли бы запретить перемещение в определенное направление или в определенную позицию (скажем, позволить x, y принимать только неотрицательные значения). Тогда бы контракт Fall() имело бы смысл переписать немного по другому

public bool Fall();
public bool CanFall { get; }


то есть в таком случае ограничение на падение задавалось бы самим состоянием частицы и никак не зависело (в самой конкретной реализации) от ограничений окружающей среды. А окружающая среда уже сама принимала решение уже основываясь на своих правилах и ограничениях.]

Теперь ваши объекты могу выглядеть следующим образом

class Sand : FallableParticle
{
    public Sand(int x, int y) : base(x, y) => C = '0';

    public override void Fall() => Y += 1;
}

class Block : Particle
{
    public Block(int x, int y) : base(x, y) => C = '#';
}


3. Класс ParticleStorage. Несмотря на то что внутри вы объявляете List сами возможности этой коллекции вы не используете. Нужно четко понимать когда делать класс мутабельным (изменяемым) а когда нет. Когда использовать иммутабельные коллекции а когда нет.

В данном случае вы не изменяете внутреннюю коллекцию частиц, а значит динамический массив (List по своей сути это именно что динамический массив, а не настоящий список) вам не нужен.

При использовании изменяемых коллекций будь крайне осторожны давать к ним внешний доступ. Метод GetParticles опасен - с вашим набором кто угодно может сделать все что им вздумается. Вместо этого можно просто реализовать интерфейс IEnumerable что позволит итерировать по коллекции но не менять ее.

Далее, сам метод GetParticleByPosition не используется по назначению. Вместо этого вы просто проверяете существование частицы а само возвращенное значение отбрасываете.

С учетом сказанного выше можно переписать этот класс немного по другому:

class ParticleStorage : IEnumerable<Particle>
{
    readonly ImmutableArray<Particle> particles;
    public ParticleStorage(IEnumerable<Particle> p) =>
        particles = p.ToImmutableArray();

    public bool IsParticleExists(int x, int y) =>
        this.Any(p => p.X == x && p.Y == y);

    public IEnumerator<Particle> GetEnumerator() =>
        ((IEnumerable<Particle>)particles).GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() =>
        ((IEnumerable)particles).GetEnumerator();
}


4. Класс Mover.

1. Название метода MoveAllParticles может ввести в заблуждение - ведь двигаться будут не все частицы. Кроме того судя по параметру и так понятно что речь идет именно о частицах а не о чем-то другом. Так что лаконичного Move будет достаточно чтобы смысл метода был ясен.

2. Метод можно упростить использовав Linq-метод OfType

class Mover
{
    public void Move(ParticleStorage ps)
    {
        foreach (var p in ps.OfType<FallableParticle>())
        {
            if (!ps.IsParticleExists(p.X, p.Y + 1))
            {
                p.Fall();
            }
        }
    }
}
Ответ написан
Пригласить эксперта
Ответы на вопрос 2
@Mylistryx
код ради кода тоже так себе идея, а на первый взгляд все ок. некоторые вещи вынес бы в конструктор мутабельный, но тоже так себе решение.
Ответ написан
@AndromedaStar
.Net - monkey
Код с точки зрения ООП бессмысленный. Ну и вообще тоже все совсем не очень хорошо. На ревью пришлось бы переписывать все заново. Но это не негатив, а просто с первого раза сложно ожидать чего-то. Вот любой алгоритм можно легко описать в процедурном стиле, но люди пользуются ООП, это дает вполне ощутимые преимущества при разработке и поддержке. Какие преимущества дает этот код?
Была бы у вас личка я бы подробнее объяснил.
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы