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

Breadth First Search not SOLID enough

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

Problem

The following code has a modified version of the Breadth First Search algorithm. It not only visits the nodes, but keeps track of the paths, so I can output various messages later. I would like to make it more reusable and cut down on some of the duplicated code. There appears to be a lot of noise in it. Here are some questions:

  • How can I refactor this code in general to adhere to more SOLID principles?



  • How can I make it so that I don't have to have a method for everything I want to do with the path (Find the shortest, find the paths with number of stops, find all paths, etc)?



  • How can I avoid the duplicated if statements when doing the console outputs?



  • Any other ideas are more than welcome?



Node class

internal class Node
{
    public string Name { get; private set; }
    public List Edges { get; private set; }

    public Node(string name)
    {
        Name = name;
        Edges = new List();
    }
}


Edge class

internal class Edge
{

    public Node TargetNode { get; private set; }
    public double Weight { get; private set; }

    public Edge(Node targetNode, double weight)
    {
        TargetNode = targetNode;
        Weight = weight;
    }
}


QueueItem class

internal class QueueItem
{

    public Node Node { get; private set; }
    public List Visited { get; private set; }

    public QueueItem(Node node, List visited)
    {
        Node = node;
        Visited = visited;
    }
}


Program class

```
class Program
{
static void Main(string[] args)
{

var a = new Node("A");
var b = new Node("B");
var c = new Node("C");
var d = new Node("D");
var e = new Node("E");

a.Edges.Add(new Edge(b, 5));
a.Edges.Add(new Edge(e, 7));
a.Edges.Add(new Edge(d, 5));
b.Edges.Add(new Edge(c, 4));
c.Edges.Add(new Edge(e, 2));
c.Edges.Add(new Edge(d, 8));
d.Edges.Add(new Edge(c, 8));
d.Edges.Add(new Edge(e, 6));
e.Edges.Ad

Solution

Cosmetics

At first glance, your post looks about right... until the Program class. But before I dig into the matter of the subject, I like to scratch the surface a bit.
Whitespace Consistency

Try to be more consistent with vertical whitespace. I think the Node and Program classes have it right:

internal class Node
{
    public string Name { get; private set; }


Naming

In general, you're using good, clear, readable names, and your naming conventions follow the official C# conventions, well done!

These ones stick out like a sore thumb though:

var a = new Node("A");
var b = new Node("B");
var c = new Node("C");
var d = new Node("D");
var e = new Node("E");


Single-letter identifiers are never a good sign. At a glance I'd recommend making an array of nodes:

var nodes = new[] 
{ 
    new Node("A"), 
    new Node("B"), 
    new Node("C"),
    new Node("D"),
    new Node("E")
};


...but this isn't ideal, building the nodes shouldn't be done here.

This is wrong though:

private static void GetAllPaths(...)


A method that's called GetXxxxx should be returning Xxxxxx - I would expect GetAllPaths to return all paths. Being void, I can already say that calling this method will have side-effects on the values I give it.
Access Modifiers

I think internal is overkill here, these types aren't hurting anyone being public. What I don't get, is why only FindAllNodes is public while every single other member of the Program class is private.

SOLID

There's quite a lot of refactoring work to be done before this code can adhere to SOLID principles.
S. Single Responsibility Principle

The Main method in the Program class is doing way too many things:

  • It builds a bunch of Node objects. Look into a builder or factory pattern to extract that functionality. This node-building code looks very frail and bug-prone, easy to get wrong and break.



  • It contains the entire functionality, and makes all function calls. Because the methods being called have side-effects, the order in which these methods are called actually matters, and will produce different results if modified: this is called temporal coupling, and should be avoided.



Each private method (oh, and the public one, too) is also doing too many things - they all "do their own thing", but then they're also responsible for outputting to the console.

The first thing to do, is to change all these void methods for functions that return a result - and let the calling code decide how it wants to output that result.

All other SOLID principles are irrelevant at this point. Why? Because you don't write code to be SOLID; you write code, and as you clean it up and refactor, it becomes SOLID. While SRP is still taking a beating, none of the other principles are applicable.

You need to extract classes, and keep the Program class - and especially anything static, to a minimum. I would start with moving the node-building to some NodeBuilder class, and the should-all-be-private methods into some PathFinder class.

Tuples

I don't like Tuple. Maybe it's just me, but whenever I used it, it seemed like a missed opportunity at making a meaningful class, or more appropriately, an immutable struct.

This:

new Tuple(path.ToString(), visited.Count, totalWeight)


Should be written like this:

Tuple.Create(path.ToString(), visited.Count, totalWeight)


But wouldn't it be clearer to have an immutable struct with PathRepresentation, VisitedCount and TotalWeight members, so that the rest of your code can call a path representation PathRepresentation instead of Item1?

Code Snippets

internal class Node
{
    public string Name { get; private set; }
var a = new Node("A");
var b = new Node("B");
var c = new Node("C");
var d = new Node("D");
var e = new Node("E");
var nodes = new[] 
{ 
    new Node("A"), 
    new Node("B"), 
    new Node("C"),
    new Node("D"),
    new Node("E")
};
private static void GetAllPaths(...)
new Tuple<string, int, double>(path.ToString(), visited.Count, totalWeight)

Context

StackExchange Code Review Q#64943, answer score: 10

Revisions (0)

No revisions yet.