HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Nested class or two separate classes?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
separatenestedtwoclassesclass

Problem

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

Solution

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 dots = new List();

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

Code Snippets

private Group group;

public Group getGroup() {
    return group;
}

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

// exercise for the OP...
private List<Dot> dots = new List<Dot>();

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

public void addDot(Dot dot) {
    dots.Add(dot);
}
public class ChangeRequest : WorkItem {
    // ....
}
class Container
{
 public class Nested
    {
        Nested() { }
    }
}

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

Context

StackExchange Code Review Q#44263, answer score: 10

Revisions (0)

No revisions yet.