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. It should look like this

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

• Magus: This looks rather like Java. C# has properties, which are quite convenient for what you're doing, and has the convention that public members are PascalCase.
• Qwert: Well I am learning OOP, and when teacher was explaining classes he used slideshow with java examples but he said its all the same. Could you maybe link me site with explanation or something please. Ty.
• svick: If possible, you should always write your code in English. That way, you can avoid situations where the code you posted somewhere won't compile, because you forgot to translate constructor from Czech. :-)

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').

• Matthew Steeples: I'm not sure your comment on scoping is correct. Your quote refers to classes. Members by default are private.
• Uri Agassi: @MatthewSteeples - apparently, it has been too long since I wrote in C# (msdn.microsoft.com/en-us/library/ms173121(v=vs.80).aspx), thanks for clearing this up.

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)