patterncppMinor
C++ Graph Implementation
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
Pointer to pair?
Similarly, for
And then just use a range-based for expression to add them all:
Pointer to bool?
You see a trend here, hopefully. In your
But you never delete it, so you're leaking that memory. Prefer:
Also, this already sets everything to
What does BFS do?
This is a
Also, again, prefer range-based-for for the push:
It's just much shorter.
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.