patterncsharpMinor
Bob the (Graph) Builder
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 :
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.
Note : The
```
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));
My graph has the following characteristics :
- It can be directional or not, implying that the
EdgebetweenNodeAandNodeBcan be traversed fromB to Aif theGraphisNonDirectional.
- The
Edgecan be "weighted", meaning that there's anintvalue 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:
Edge:
If I understand correctly that the point of having
This way,
Equals(Node other)will throwNullReferenceExceptionwhen called withnull
GetHashCode()should only returnName.GetHashCode()- the multiplication can cause an overflow and two distinct values of the original hash can get mapped to the same value
ToString()instead ofthis.GetType().Namefor the name of the type you can usenameof(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.