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

Bob the (Graph) Builder

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

Problem

I'm starting a project in which I'll generate random graphs and use algorithms to solve them. The first necessary step is obviously to build a graph.

My graph has the following characteristics :

  • It can be directional or not, implying that the Edge between NodeA and NodeB can be traversed from B to A if the Graph is NonDirectional.



  • The Edge can be "weighted", meaning that there's an int value linked to an edge, which will be used for some graph algorithms.



At the moment, I have mostly tested the non directional weighted graph, so it's the one I'll put for review. I hav removed the header comments from the class/methods as I have multiple classes to show.

public class Node : IEquatable
{
    public string Name { get; }

    public Node(string name)
    {
        if (String.IsNullOrWhiteSpace(name)) throw new ArgumentException("Name cannot be null or empty", nameof(name));

        Name = name;
    }

    public override bool Equals(object obj)
    {
        return Equals(obj as Node);
    }

    public bool Equals(Node other)
    {
        return Name == other.Name;
    }

    public override int GetHashCode()
    {
        return Name.GetHashCode() * 23; 
    }

    public override string ToString()
    {
        return $"({this.GetType().Name}) - Name : {Name}";
    }
}


Note : The PriorityQueueNode base class is used in the algorithms, but it is not used anywhere in this review. The AssociationId is used in case of a non directional graph, it is used to see if two edges are from the same... association (A to B and B to A).

```
public class Edge : PriorityQueueNode, IEquatable
{
private readonly Lazy computedHashCode;

public Guid AssociationId { get; }
public Node From { get; }
public Node To { get; }

public Edge(Node from, Node to, Guid associationId)
{
if (to == null) throw new ArgumentNullException(nameof(to));
if (from == null) throw new ArgumentNullException(nameof(from));

Solution

Node:

  • Equals(Node other) will throw NullReferenceException when called with null



  • GetHashCode() should only return Name.GetHashCode() - the multiplication can cause an overflow and two distinct values of the original hash can get mapped to the same value



  • ToString() instead of this.GetType().Name for the name of the type you can use nameof(Node) which is actually a string literal



Edge:

If I understand correctly that the point of having Guid AssociationId that is set on creation is to label directed edges x -> y and y -> x with the same Guid then I would suggest an alternative implementation:

public bool ConnectsSameNodes(Edge edge)
{
    if (edge == null)
    {
        throw new ArgumentNullException(nameof(edge));
    }

    return (From.Equals(edge.From) && To.Equals(edge.To)) ||
           (From.Equals(edge.To) && To.Equals(edge.From));
}


This way, Guid AssociationId can be removed from the class completely.

Code Snippets

public bool ConnectsSameNodes(Edge edge)
{
    if (edge == null)
    {
        throw new ArgumentNullException(nameof(edge));
    }

    return (From.Equals(edge.From) && To.Equals(edge.To)) ||
           (From.Equals(edge.To) && To.Equals(edge.From));
}

Context

StackExchange Code Review Q#115526, answer score: 3

Revisions (0)

No revisions yet.