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

Depth First Search and Breadth First Search in C++

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

Problem

I am trying to learn DFS and BFS. However, I just want to make sure that I am not doing anything wrong. Would you please determine if the BFS and DFS functions are correct?

#include 
#include 
#include 
#include 
using namespace std;

class Graph
{
private:
    int num_of_vertices;
    vector *Adj;
public:
    Graph(int V);
    void addEdge(int from, int to);
    void BFS(int Start);
    void DFS(int Start);
};

Graph::Graph(int V)
{
    this->num_of_vertices = V;
    Adj = new vector[V];
}
void Graph::addEdge(int from, int to)
{
    Adj[from].push_back(to);
}

void Graph::BFS(int Start)
{
    bool* visited = new bool[this->num_of_vertices]();
    queue queue;
    queue.push(Start);
    vector::iterator i;
    cout num_of_vertices]();
    stack stack;
    stack.push(Start);
    vector::iterator i;
    cout << "DFS: ";
    while(!stack.empty())
    {
        int top = stack.top();
        cout << top << " ";
        stack.pop();
        visited[top] = true;
        for(i = Adj[top].begin(); i != Adj[top].end(); i++)
        {
            if (!visited[*i])
                stack.push(*i);
        }
    }
    cout << endl;
}

Solution

The implementation of the BFS and DFS algorithms themselves seems correct(but it is a good practice to write unit-tests to make sure that it works as intended, so you should do it). However, your code in general is not correct. It definitely leaks memory:

-
In the constructor, Adj is allocated: Adj = new vector[V];. However, it is never deleted. You can do it in a destructor.

-
The same is true for the visited array(you should delete it at the end of the DFS and BFS member-functions).

Actually, there is a much easier way to deal with this issue: do not use pointers and dynamic memory allocation. I do not see any point in visited being allocated dynamically. Using an std::vector is much better(std::vector visited(num_of_vertices)). The same holds true for the Adj member-variable.

One more thing: you should try to keep the scope of variables as narrow as possible. There is no need to declare the vector::iterator i at the beginning of the function. You can do it just inside the for loop where it is used.

The last thing: using namespace std; is a bad practice. It pollutes the global namespace.

Context

StackExchange Code Review Q#82476, answer score: 8

Revisions (0)

No revisions yet.