Problem
I’ve developed program, which may work with graphs and its edges (connections between vertices):
/*
Oleg Orlov, 2012(c), generating randomly adjacency matrix and graph connections
*/
using System;
using System.Collections.Generic;
class Graph
{
internal int id;
private int value;
internal Graph[] links;
public Graph(int inc_id, int inc_value)
{
this.id = inc_id;
this.value = inc_value;
links = new Graph[Program.random_generator.Next(0, 4)];
}
}
class Program
{
private const int graphs_count = 10;
private static List<Graph> list;
public static Random random_generator;
private static void Init()
{
random_generator = new Random();
list = new List<Graph>(graphs_count);
for (int i = 0; i < list.Capacity; i++)
{
list.Add(new Graph(i, random_generator.Next(100, 255) * i + random_generator.Next(0, 32)));
}
}
private static void InitGraphs()
{
for (int i = 0; i < list.Count; i++)
{
Graph graph = list[i] as Graph;
graph.links = new Graph[random_generator.Next(1, 4)];
for (int j = 0; j < graph.links.Length; j++)
{
graph.links[j] = list[random_generator.Next(0, 10)];
}
list[i] = graph;
}
}
private static bool[,] ParseAdjectiveMatrix()
{
bool[,] matrix = new bool[list.Count, list.Count];
foreach (Graph graph in list)
{
int[] links = new int[graph.links.Length];
for (int i = 0; i < links.Length; i++)
{
links[i] = graph.links[i].id;
matrix[graph.id, links[i]] = matrix[links[i], graph.id] = true;
}
}
return matrix;
}
private static void PrintMatrix(ref bool[,] matrix)
{
for (int i = 0; i < list.Count; i++)
{
Console.Write("{0} | [ ", i);
for (int j = 0; j < list.Count; j++)
{
Console.Write(" {0},", Convert.ToInt32(matrix[i, j]));
}
Console.Write(" ]rn");
}
Console.Write("{0}", new string(' ', 7));
for (int i = 0; i < list.Count; i++)
{
Console.Write("---");
}
Console.Write("rn{0}", new string(' ', 7));
for (int i = 0; i < list.Count; i++)
{
Console.Write("{0} ", i);
}
Console.Write("rn");
}
private static void PrintGraphs()
{
foreach (Graph graph in list)
{
Console.Write("rnGraph id: {0}. It references to the graphs: ", graph.id);
for (int i = 0; i < graph.links.Length; i++)
{
Console.Write(" {0}", graph.links[i].id);
}
}
}
[STAThread]
static void Main()
{
try
{
Init();
InitGraphs();
bool[,] matrix = ParseAdjectiveMatrix();
PrintMatrix(ref matrix);
PrintGraphs();
}
catch (Exception exc)
{
Console.WriteLine(exc.Message);
}
Console.Write("rnrnPress enter to exit this program...");
Console.ReadLine();
}
}
The example there is using the const value, but you could erase const and fill Random int.
At the ideone as you see the result is ok and are able to see the generated matrix (result):
0 | [ 0, 0, 0, 1, 0, 0, 0, 0, 1, 1, ] 1 | [ 0, 0, 0, 1, 1, 0, 0, 0, 1, 0, ] 2 | [ 0, 0, 1, 0, 1, 0, 0, 0, 1, 1, ] 3 | [ 1, 1, 0, 0, 0, 0, 0, 1, 0, 0, ] 4 | [ 0, 1, 1, 0, 0, 1, 0, 1, 0, 0, ] 5 | [ 0, 0, 0, 0, 1, 0, 0, 1, 1, 1, ] 6 | [ 0, 0, 0, 0, 0, 0, 1, 1, 0, 1, ] 7 | [ 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, ] 8 | [ 1, 1, 1, 0, 0, 1, 0, 0, 0, 0, ] 9 | [ 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, ] ------------------------------ 0 1 2 3 4 5 6 7 8 9 Graph id: 0. It references to the graphs: 8 Graph id: 1. It references to the graphs: 4 8 Graph id: 2. It references to the graphs: 4 2 4 Graph id: 3. It references to the graphs: 1 0 1 Graph id: 4. It references to the graphs: 7 5 Graph id: 5. It references to the graphs: 9 7 Graph id: 6. It references to the graphs: 6 9 Graph id: 7. It references to the graphs: 3 6 Graph id: 8. It references to the graphs: 5 2 5 Graph id: 9. It references to the graphs: 2 0 9 Press enter to exit this program...
Solution
class Graph
What this class represents is not a whole graph, it’s a single vertex (or node). Because of that, you should rename this class to either Vertex or Node.
internal int id;
You shouldn’t have non-private fields, use properties for that. For more information about reasons, read Jon Skeet’s article Why properties matter.
Also, I don’t understand why are you marking fields internal, since the whole class is already internal (that’s the default for top-level classes). You should mark them public, so that if you decide to make the class public in the future, you don’t have to change the fields (or rather properties, see previous point) too.
One more thing: you should also change the name to Id, that’s the naming convention used for non-private members.
class Program
All methods of this class are static, so you should make the whole class static. Although it might also make sense to put all the graph-related code into a separate (non-static class). A good name for this new class might be Graph.
private const int graphs_count = 10;
This should be static readonly, rather than const. const fields should be used only for values that you know will never change. This is most important if you’re writing a library, but it’s a good habit to get used to it everywhere.
private static List<Graph> list;
list is a very unclear name, a better name would be something like graphs (or nodes after you fix the name of the class Graph).
public static Random random_generator;
Again, the naming convention for non-private members is something like RandomGenerator, you should change that. And again, public fields should be changed to public properties.
private static void Init()
private static void InitGraphs()
Having methods that have to be called before a class can be used is a sign of bad design. You should use constructor for that.
Graph graph = list[i] as Graph;
There is no need for the as here, the type if list[i] is already Graph.
list[i] = graph;
This line doesn’t do anything, you should remove it. In C#, classes are reference types, so if you change members of the local variable graph here, list[i] will be also changed (because it refers to the same object).
private static bool[,] ParseAdjectiveMatrix()
This is again a very confusing name. There is no such thing as “adjective matrix”, it’s called adjacency matrix, so you should rename this method. Also, “parsing” usually means reading some string. A better name for this method would then be CreateAdjacencyMatrix.
int[] links = new int[graph.links.Length];
There doesn’t seem to be any reason for this array, you could simplify your code to:
for (int i = 0; i < graph.links.Length; i++)
{
int linkedId = graph.links[i].id;
matrix[graph.id, linkedId] = matrix[linkedId, graph.id] = true;
}
Or even better, use foreach:
foreach (var linked in graph.links)
{
matrix[graph.id, linked.id] = matrix[linked.id, graph.id] = true;
}
private static void PrintMatrix(ref bool[,] matrix)
You should get rid of the ref here. ref is useful if you want to change some variable of the caller (and not just its contents) or in very rare cases when you want to avoid copying a large value type. But matrix is an array, which is a reference type, and you don’t change it, so the ref has no use here.
Console.Write(" ]rn");
You should use Console.WriteLine(" ]") here instead. This applies to other places in your code where you use rn too.