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

Algorithm to group connected graphs

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

Problem

I am working on an algorithm, which should group connected Arcs. The situation is described via code examples below.

I have Arcs:

public class Arc
{
    public Point StartPoint { get; set; }
    public Point EndPoint { get; set; }
    public bool Visited { get; set; }
    public Arc Successor { get; set; }
    public Arc Predecessor { get; set; }
}


that consist of Points:

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }

    public override bool Equals(Object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        return obj.GetType() == GetType() && Equals((Point)obj);
    }

    protected bool Equals(Point other)
    {
        return X.Equals(other.X) && Y.Equals(other.Y);
    }

    public override int GetHashCode()
    {
        unchecked
        {
        return (X.GetHashCode() * 397) ^ Y.GetHashCode();
        }
    }
}


Given these Points and resulting Arcs:

var p1 = new Point {X = 0, Y = 0};
var p2 = new Point { X = 0, Y = 1 };
var p3 = new Point { X = 0, Y = 2 };
var p4 = new Point { X = 1, Y = 0 };
var p5 = new Point { X = 1, Y = 1 };

var arc1 = new Arc { StartPoint = p1, EndPoint = p2};
var arc2 = new Arc { StartPoint = p4, EndPoint = p5};
var arc3 = new Arc { StartPoint = p2, EndPoint = p3};


I would like to end up with groups of connected Arcs as a list of list:

var arcListOfList = new List>();


This is the crude algorithm I came up with so far:

```
foreach (var arc in arcList)
{
var successor = arcList.FirstOrDefault(x => x.StartPoint.Equals(arc.EndPoint));
var predecessor = arcList.FirstOrDefault(x => x.EndPoint.Equals(arc.StartPoint));
arc.Successor = successor;
arc.Predecessor = predecessor;
}

Arc next;
do
{
next = arcList.FirstOrDefault(x => x.Visited == false);
var temp = new List();

if (next == null) continue;
do
{
next.Visited = true;
temp.Add(next);

Solution

I'm going to leave off suggesting anything about the algorithm for now as I don't fully understand what you want from it. Instead, I'm going to talk about your Point class in laborious depth...

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }


I would argue that a point should be immutable - i.e. it doesn't change after you've created it. I would implement with full readonly properties:

private readonly int x;
public int X { get { return x; } }

private readonly int y;
public int Y { get { return y; } }

public Point(int x, int y)
{
   this.x = x;
   this.y = y
}


However, most people would let you off with private set auto properties:

public int X { get; private set; }


Let's look at your Equals implementations

public override bool Equals(Object obj)
{
    if (ReferenceEquals(null, obj)) return false;
    if (ReferenceEquals(this, obj)) return true;
    return obj.GetType() == GetType() && Equals((Point)obj);
}

protected bool Equals(Point other)
{
    return X.Equals(other.X) && Y.Equals(other.Y);
}


Firstly, bool Equals(Point other) -> that's IEquatable if made public so your class should implement that:

public class Point : IEquatable


Now, simplify the object.Equals override:

public override bool Equals(Object obj)
{
    return Equals(obj as Point);
}


And sharpen up the IEquatable.Equals:

public bool Equals(Point other)
{
    return other != null
            && other.X == X
            && other.Y == Y;
}


We should also override the == and != operators so that we're sure we are consistent with out equality:

public static bool operator ==(Point left, Point right)
{
    if (object.ReferenceEquals(left, null))
    {
        return object.ReferenceEquals(right, null);
    }

    return left.Equals(right);
}

public static bool operator !=(Point left, Point right)
{
    if (object.ReferenceEquals(left, null))
    {
        return !object.ReferenceEquals(right, null);
    }

    return !left.Equals(right);
}


Then you have a nice immutable class for your Point with consistent equality behaviour... Phew!

Edit

As EBrown pointed out in the comments, Point is a perfect fit for being a struct. It's less than 16 bytes, immutable and (0, 0) is a perfectly valid value (i.e. unitialized).

Code Snippets

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }
private readonly int x;
public int X { get { return x; } }

private readonly int y;
public int Y { get { return y; } }

public Point(int x, int y)
{
   this.x = x;
   this.y = y
}
public int X { get; private set; }
public override bool Equals(Object obj)
{
    if (ReferenceEquals(null, obj)) return false;
    if (ReferenceEquals(this, obj)) return true;
    return obj.GetType() == GetType() && Equals((Point)obj);
}

protected bool Equals(Point other)
{
    return X.Equals(other.X) && Y.Equals(other.Y);
}
public class Point : IEquatable<Point>

Context

StackExchange Code Review Q#97129, answer score: 3

Revisions (0)

No revisions yet.