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

C++ Graph Implementation

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

Problem

My data set is a list of Edges which are passed as a pair of integers. Based on that, I have the following graph implementation for BFS. Could someone please review my code and comment on errors/omissions/efficiency and other points.

#include 
#include 
#include 

class Graph {

private:
    int num_of_vertices;
    std::vector* Adj;

public:
    Graph(int V){

        this->num_of_vertices = V;
        Adj = new std::vector[V];
    }

    void addEdge(std::vector* > edge_list)
    {
        for(auto it = edge_list.begin(); it != edge_list.end(); it++) {
            std::pair* p = *it;
            Adj[p->first].push_back(p->second);
        }
    }
    void BFS(int start)
    {
        bool* visited = new bool[this->num_of_vertices]();

        for(int i=0;i queue;
        queue.push(start);
        std::vector::iterator i;

        while(!queue.empty())
        {
            start = queue.front();

            visited[start] = true;
            queue.pop();
            for (i = Adj[start].begin(); i != Adj[start].end(); i++)
            {
                if (!visited[*i])
                    queue.push(*i);
            }
        }

    }

};

Solution

Pointer to vector?

You have two members: a pointer to a vector (which you new, but never delete, which leads to several other problems) and its size. But you're already using a vector, so just do it twice:

std::vector> adjacency_vector;

Graph(int num_vertices)
: adjacency_vector(num_vertices)
{ }


Pointer to pair?

Similarly, for addEdge, take a vector of pairs - not a vector of pointers to pairs - and by reference to const:

void addEdge(std::vector> const& edge_list)


And then just use a range-based for expression to add them all:

for (auto const& edge : edge_list) {
    adjacency_vector[edge.first].push_back(edge.second);
}


Pointer to bool?

You see a trend here, hopefully. In your BFS, you start with:

bool* visited = new bool[this->num_of_vertices]();


But you never delete it, so you're leaking that memory. Prefer:

std::vector visited(adjacency_vector.size());


Also, this already sets everything to false, so you don't have to manually. Though your original implementation did as well.

What does BFS do?

This is a void function, that does something with local variables queue and visited - and then what? What as the outside observer do I get? This function needs to return... something of value to the caller. Otherwise, it's just spending time doing nothing.

Also, again, prefer range-based-for for the push:

for (auto child : adjacency_vector[start]) {
    if (!visited[child]) {
        queue.push(child);
    }
}


It's just much shorter.

Code Snippets

std::vector<std::vector<int>> adjacency_vector;

Graph(int num_vertices)
: adjacency_vector(num_vertices)
{ }
void addEdge(std::vector<std::pair<int, int>> const& edge_list)
for (auto const& edge : edge_list) {
    adjacency_vector[edge.first].push_back(edge.second);
}
bool* visited = new bool[this->num_of_vertices]();
std::vector<bool> visited(adjacency_vector.size());

Context

StackExchange Code Review Q#114304, answer score: 6

Revisions (0)

No revisions yet.