Here is my code for implementing Graph using adjacency matrix. The code should be Object-oriented and there is a method isPath()
checking if there is a connection between two nodes. Some advice?
#include <iostream>
#include <cstdlib>
#include <fstream>
using namespace std;
class Graph
{
protected:
int value;
int **graphm;
public:
Graph(int value)
{
this->value = value;
graphm = new int*[value];
int k, j;
for (k = 0; k < value; k++)
{
graphm[k] = new int[value];
for (j = 0; j < value; j++)
{
graphm[k][j] = 0;
}
}
}
void newEdge(int head, int end)
{
if (head > value || end > value || head < 0 || end < 0)
{
cout << "Invalid edge!\n";
}
else
{
graphm[head - 1][end - 1] = 1;
graphm[end - 1][head - 1] = 1;
}
}
void display()
{
int i, p;
for (i = 0; i < value; i++)
{
for (p = 0; p < value; p++)
{
cout << graphm[i][p] << " ";
}
cout << endl;
}
}
void isPath(int head, int end)
{
int k, o;
ofstream fullstore("Test.txt");
cout << graphm[head - 1][end - 1];
cout << endl;
if (!fullstore.is_open())
{
cout << "File can't be open";
}
if (graphm[head - 1][end - 1] == 1)
{
cout << "There is an edge between " << head << " and " << end << "\n";
fullstore << head << ", " << end;
fullstore.close();
}
else
{
cout << "Edge not found\n";
}
}
};
int main()
{
int vertex, numberOfEdges, i, head, end;
cout << "Enter number of nodes: ";
cin >> vertex;
numberOfEdges = vertex * (vertex - 1);
Graph g1(vertex);
for (int i = 0; i < numberOfEdges; i++)
{
cout << "Enter edge ex.1 2 (-1 -1 to exit): \n";
cin >> head >> end;
if ((head == -1) && (end == -1))
{
break;
}
g1.newEdge(head, end);
}
g1.display();
cout << endl;
g1.isPath(1, 3);
return 0;
}
2 Answers 2
Avoid using namespace std;
.
Why are you using raw pointers? Use smart pointers, or (better yet) standard containers like std::vector
. Since vector
constructs its elements, much of your initialization code can be eliminated.
Make use of the member initializer list in your constructor. There's no real difference with fundamental types like int
, but with more complicated objects it can be a big help or outright necessity (to initialize a reference).
Graph(int value): value(value) {
(That will initialize the member value
with the parameter value
.) Then get rid of the this->value = value
line.
Define variables as late as possible, with their first use if possible. Make use of this in your for
statements.
for (int k = 0; k < value; ++k)
In newEdge
, your parameter validation accepts 0 as a valid edge index, but you then subtract one from it for the subscripting, resulting in Undefined Behavior and a possible crash.
display
and isPath
should be const (void display() const
. isPath
does not validate its parameters like newEdge
does.
Avoid using namespace std;
To know why:
Memory management
delete
Each time you write new
think about delete
. (And in this case, about delete []
). If you manually allocate memory, you should care about freeing otherwise welcome to memory leaks.
smart pointers
If you to have to use pointer (and ask yourself, "do I really need to?") try to use smart pointers instead of raw pointers. (and if no choice, use them correctly)
- It's the Standard way to go
- There are many advantages
- For almost zero-overhead
Adopt Standards
Containers
Choose and use proper types and containers.
Algorithms
It's really important to know your algorithms.
Misc
Const-correctness
When a parameter doesn't have to change, make it const.
Includes
Include only requested header. Why including <cstdlib>
here ?
iostream
Don't close manually istream if you don't have to.
std::end
Avoid using std::endl
. It send '\n'
to the steam and then flush the steam. If you need to flush, do it explicitly.
Inputtings and parameters
Always validate inputs. Don't assume that users don't try to break your program.
Variables
Define variable in the closest scope possible and use names that make sense.