Для начала сделаю оговорку что основным языком программирования для меня является 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();
}
}
}
}
