D
D
Dmitry2021-09-20 19:15:51
OOP
Dmitry, 2021-09-20 19:15:51

Is this structuring of code correct in the context of OOP?

Question for people who understand OOP. I'm getting into OOP on my own (I'm reading the book "Principles, Patterns and Methods of Agile Development in C#" and reworking my old simple console programs, trying to bring them to what I understood as OOP). Due to the fact that I don’t have programming experts in my environment (not to mention OOP), I simply don’t have anyone to ask if I’m doing the right thing and what needs (if necessary) to be corrected. So I decided to dump my works on public display, hoping to get criticism or advice.

First, in general, I have an application that simulates the behavior of sand, I decided to rewrite it by drawing up a UML diagram (which, however, I only finished with the code itself). The diagram looks like this:
6148b29b2d76e166640408.png

the code of all elements is the same:

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);
        }
    }
}

Answer the question

In order to leave comments, you need to log in

3 answer(s)
F
Foggy Finder, 2021-09-22
@nosochek

To begin with, I’ll make a reservation that the main programming language for me is F #, which is primarily functional, but I’ll leave a couple of general tips, recommendations and just thoughts, including on OOP.
1. Particle class . Remember - there should not be any public fields in the classes. This is a gross violation of encapsulation. Accordingly, the GetChar method does not make sense, because there is already access to information.
As for the GetPosition method, the existing class heirs do not override it, which means there is no need to make it virtual. In addition, all its uses can be easily replaced with property calls and creating a tuple for such purposes, in my opinion, is superfluous.

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 interface . Its meaning is not clear. What should it reflect? Some object that can fall? If yes, then
a) such an interface would be appropriate if certain behavior is inherent in different, unrelated types.
b) Strong dependency on ParticleStorage . [ Edit: This means that the CanFall contract parameter is ParticleStorage ] The interface becomes completely inflexible.
If your intention was to separate the type of particle from the type of particle that can fall , then perhaps a better solution would be to create a new abstract class derived from Particle?
abstract class FallableParticle : Particle
{
    protected FallableParticle(int _x, int _y) : base(_x, _y)
    {
    }

    public abstract void Fall();
}

---
[ Edit : Whether it is better to use an abstract class or an interface in many situations, there is no unambiguously correct answer. It all depends on future use. There is no multiple inheritance in C# (which is great) and if there is a need to describe different behaviors (falling, burning) then, of course, using interfaces seems more appropriate.
So it all depends on the context and the intended use, extension. In other words - if another functionality (burning) is not supposed to be implemented in the near future, then leaving the abstract class seems like an adequate solution. If it is planned, even if it is for the future, it is better to use interfaces.]
---
Please note: I completely removed the CanFall method- He's not needed. A falling particle can fall by definition. If her ability to move is determined from outside, then the check should be there.
[ Edit : In this case, the move check in the Mover class
if (f.CanFall(ps))
{
    f.Fall();
}

this is a good decision. Mover is a concrete implementation of some kind of abstract movement in an abstract space. Everything is fine here and an additional entity would be an over-complication.
The checks themselves can be multi-level. For example, we could prohibit movement in a certain direction or position (say, allow x, y to take only non-negative values). Then it would make sense to rewrite the Fall() contract a little differently
public bool Fall();
public bool CanFall { get; }

that is, in this case, the limitation on the fall would be set by the state of the particle itself and would in no way depend (in the most specific implementation) on the limitations of the environment. And the environment already made a decision based on its own rules and restrictions.]
Now your objects can look like this
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 class . Despite the fact that inside you declare a List , you do not use the capabilities of this collection yourself. You need to clearly understand when to make a class mutable (changeable) and when not. When to use immutable collections and when not.
In this case, you don't change the internal collection of particles, which means you don't need a dynamic array ( List is essentially a dynamic array, not a real list).
When using mutable collections, be extremely careful about making them available externally. The GetParticles method is dangerous - with your set, anyone can do whatever they want. Instead, you can simply implement the IEnumerable interfacewhich will allow you to iterate over the collection but not change it.
Further, the GetParticleByPosition method itself is not used for its intended purpose. Instead, you simply test for the existence of the particle and discard the return value itself.
Given the above, you can rewrite this class a little differently:
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 class .
1. The name of the MoveAllParticles method can be misleading - after all, not all particles will move. In addition, judging by the parameter, it is already clear that we are talking about particles and not about something else. So a laconic Move will be enough to make the meaning of the method clear.
2. The method can be simplified by using the Linq method 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();
            }
        }
    }
}

M
Mylistryx, 2021-09-21
@Mylistryx

code for the sake of code is also a so-so idea, but at first glance everything is ok. some things would be taken out to the mutable constructor, but also a so-so solution.

A
AndromedaStar, 2021-09-22
@AndromedaStar

Code from an OOP point of view is meaningless. Well, in general, too, everything is not very good at all. The review would have to rewrite everything again. But this is not a negative, but simply the first time it is difficult to expect something. Here any algorithm can be easily described in a procedural style, but people use OOP, this gives quite tangible advantages in development and support. What are the benefits of this code?
If you had a personal I would explain in more detail.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question