4
\$\begingroup\$

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;
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

Avoid using namespace std;

To know why:

  • Read this SO thread
  • Check Standard C++ recommendations: here, here
  • Read this blog

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)

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.