Nested class or two separate classes?

Question

I am making this program that is going to calculate distance from randomly generated dots (with x and y coordinates) to 5 bigger ones and then its going to put each smaller dot into the group with bigger one that is nearest to it. So, i am making this with classes and every dot is made as an object. I made 2 arrays of objects. One holds the bigger dots and other one smaller dots. I also made 2 separate classes. I am not sure if this should be done with nested classes/subclasses or my concept is ok.

My classes:

class Groups{
    int x;
    int y;
    int brush; //i use this just for groups to have different color when drawing
    public Groups(int a, int b, int brush)
    {
        this.x = a;
        this.y = b;
        this.brush = brush;
    }
    public int getX()
    {
        return this.x;
    }
    public int getY()
    {
        return this.y;
    }
    public int getBrush()
    {
        return this.brush;
    }
}

public class Dots{
    int x;
    int y;
    int group;

    public Dots(int x, int y)
    {
        this.x = x;
        this.y = y;
        this.group = 0;
    }

    public int getX()
    {return this.x;}

    public int getY()
    {return this.y;}

    public int getGroup()
    {return this.group;}

    public void setGroup(int x)
    {this.group = x;}
}

Answer 1

Naming

Class names in general should be singular - Group, Dot.

If you have a constructor which sets coordinates x and y, call the parameters it gets x and y, not a and b... Also, I would refrain from single letter names, although, in this case, x and y seem ok, since they represent coordinates.

Scoping

You declared members, and implemented getters to them which is fine (although you used the wrong idiom for C#), but you failed to declare the members as private. This means that these members are internal, and can be accessed outside the class:

Depending on the context in which a member declaration occurs, only certain declared accessibilities are permitted. If no access modifier is specified in a member declaration, a default accessibility is used. Top-level types, which are not nested in other types, can only have internal or public accessibility. The default accessibility for these types is internal.

Edit - this is, of course, wrong (thanks @Matthew) - I got confused with java), the default access modifier for members in C# is private. I still stand by my recommendation to be explicit with private, to be more readable, and compliant with coding conventions.

Object Oriented

For some reason Dot has a property called Group which is... an int?? What did you intend this property to do? If you want it to hold a Group, its type should be Group:

private Group group;

public Group getGroup() {
    return group;
}

public void setGroup(Group group) {
    this.group = group;
}

I also guess the brush is also an instance of a class (and not an int), and should be declared as such:

private Brush brush;

// exercise for the OP...

Also, what is the role of x and y for Group? I should have guessed that Group will hold some Dots instead...

private List<Dot> dots = new List<Dot>();

public Enumerable<Dot> getDots() {
    return dots;
}

public void addDot(Dot dot) {
    dots.Add(dot);
}

What are nested classes? What are subclasses?

You ask whether you should use subclasses of nested classes, but your example shows neither.

A subclass is a class which inherits from another class, and then looks and behaves just like it, plus some other functionality, which makes it a special case of its parent:

public class ChangeRequest : WorkItem {
    // ....
}

In OO design this is expressed as an is a relationship - "ChangeRequest is a WorkItem"

A nested class is a class which is declared inside another class, it does not inherit anything from it, though it might have special permissions to access its enclosing class:

class Container
{
 public class Nested
    {
        Nested() { }
    }
}

Container.Nested nest = new Container.Nested();

As you can see, the relation between Group and Dot doesn't seem to fall under any of these categories, and they should be written as separate classes, each having a property pointing to the other ('Dot has a Group' and 'Group has Dots').

Answer 2

I am not going to repeat the points of Uri Agassi.

Both of your classes have a considerable amount of common code and both represent some shape that has to be drawn. Therefore I suggest extracting the common code to an abstract base class named Shape. A Shape has a center and a brush:

public abstract class Shape
{
    public Shape(Point center, Brush brush)
    {
        _center = center;
        _brush = brush;
    }

    private Point _center;
    public Point Center { get { return _center; } }

    protected Brush _brush;
    public Brush Brush { get { return _brush; } }
}

As you can see, I am using a Point structure for the center. The Graphics object supports these points and they can be assigned with just one statement Point p = dot.Center;. The System.Drawing.Point structure has also a set of useful methods and operator overloads.

The Group class inherits Shape and passes on the center and the brush to the base constructor:

public class Group : Shape
{
    public Group(Point center, Brush brush)
        : base(center, brush)
    {
    }
}

Group adds no more members, as Shape has all it needs.

The Dot class inherits Shape as well and adds a Group property. Since we probably don't know to which group a dot belongs when it is created, we initialize the brush as black.

When we assign a group to the dot, we automatically take over the group's brush, since a dot has always the same color as the group it belongs to.

public class Dot : Shape
{
    public Dot(Point center)
        : base(center, Brushes.Black)
    {
    }

    private Group _group;
    public Group Group {
        get { return _group; }
        set
        {
            _group = value;
            _brush = value == null ? Brushes.Black : value.Brush;
        }
    }
}

I also used properties instead of accessor methods.

---

Possible extensions: You could add a radius to the Shape class. This would allow you to use the same method for drawing the groups as well as the dots. Add an abstract Radius property to Shape:

public abstract int Radius { get; }

and implement it in Group:

public override int Radius { get { return 5; } }

and in Dot:

public override int Radius { get { return 2; } }

Now you can write the drawing method which works for both shapes like this:

private void DrawShape(Graphics g, Shape shape)
{
    g.FillEllipse(shape.Brush,
        shape.Center.X - shape.Radius, shape.Center.Y - shape.Radius,
        2 * shape.Radius, 2 * shape.Radius);
}

Answer 3

for OOP practice - its best to go from small to big.

i.e.: a dot is x,y, the group is a collection of dot.

And you are correct - regardless of syntax (ie programming language) this concept of nested/inheritance (aka subclass) is sometimes harder to pick from.

the bottom line of OOP though is that you try and reduce redundancy.

(similar to person -> student / teacher)

Possible solution would be make Dot a class, make Group uses an array of Dot with Brush (because the Group is always 5 dots of x,y)