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

Implementation of Dijkstra's algorithm to find the shortest path in graph

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

Problem

This is my implementation of Dijkstra's algorithm in C++.

Please clarify

  • Whether the class design is proper or if it needs to be improved.



  • I am just hard coding the vertex based on the priority of the vertex. Is there is any other way to implement it?



  • Please provide some suggestions to optimize the code.



class Vertex
{
public:
    int m_id;
    unsigned int m_cost;
    bool m_isVisited;

    std::map m_edgeList; // map to store neighbour nodes and cost to reach them

    void findMinDistance(std::vector&);

    Vertex(int p_id)
    {
        m_id= p_id;
        if(p_id == 0)
        {
            m_cost=0;
        }
        else
        {
            m_cost=UINT_MAX; // Assigning Infinite cost
        }
        m_isVisited=false;
    }

    Vertex()
    {           
    }
};

void Vertex::findMinDistance(std::vector &p_vertex)
{

    m_isVisited= true;
    // Looping throgh the neighbour list and update cost 
    for(std::map::iterator iter = m_edgeList.begin();
        iter!=m_edgeList.end(); iter++)
    {
        //If cost of picked neighbour is greater than current_vertex+new cost then replace cost
        if ( p_vertex.at(iter->first).m_isVisited == false && p_vertex.at(iter->first).m_cost > m_cost+(iter->second))
        {
            p_vertex.at(iter->first).m_cost=m_cost+iter->second;

            std::coutfirst).m_id;
            std::coutfirst).m_cost;          
            std::coutfirst).m_cost;
        }
    }
}

int main()
{
    std::vector l_vertex;

    for(int i=0; i::iterator iter = l_vertex.begin(); iter != l_vertex.end(); iter++)
{
    (*iter).findMinDistance(l_vertex);
}

for(int i=0;i<6;i++)
std::cout<<"\n\nshortest "<<i<<" "<<l_vertex.at(i).m_cost;

return 0;
}

Solution

This is a bad idea:

class Vertex
{
public:
    int m_id;
    >>> unsigned int m_cost;
    >>> bool m_isVisited;


You are conflating two things into a single class. The concept of a vertex and the concept of performing Dijkstra's algorithm on the node. You should separate the two issues into their own classes.

The main problem with this technique is that you need to reset the graph before use. ie. you can not run your algorithm twice in a row. You must reset the state of the whole graph before it can be re-used. Look up the visitor pattern.

You really want a constructor that does no initialization?

Vertex()
{           
}


The POD members will be left in an undefined state.

Your have void findMinDistance(std::vector&); in the vertex? Find min distance to where?

Use the new C++ range based for:

for(std::map::iterator iter = m_edgeList.begin();
    iter!=m_edgeList.end(); iter++)

// can be rewritten as:
for(auto& edge: m_edgeList)


Overall I don't think you are actually implement Dijkstra's algorithm.
Dijkstra algorithm

The algorithm should look like this (this is pseudo code).

The classic part is the frontier list (I called it boundryList) and the set of vertex we have found the shortest route to.

RouteCostObject findRout(Graph& g, Vertex& start, Vectex& end)
 {
      OrderedListByDistance    boundryList;
      SetOfVertex              foundVertixes;

      boundryList.push_back([start, 0, []]);

      while(!boundryList.empty())
      {
          // Remove the top items from the list.
          (vertex, cost, route) = boundryList.top();       // Note: boundryList is sorted
          boundryList.pop();

          // If we have reached the end.
          // This is the shortest route.
          if (vertex == end)
          {    return [cost, route];
          }

          // Check if we have already found the shortest distance
          // to this vertex.
          if (foundVertixes.find(vertex) != vertex.end())
          {
              continue;
          }
          foundVertixes.insert(vertex);

          // Add all edges to the boundary list.
          for(auto& edge: vertex.edges)
          {
              boundryList.push_back([edge.dst, cost + edge.cost, route + edge]);
          }
      }
      // No route from start to end
      return [infinity, []];
 }

Code Snippets

class Vertex
{
public:
    int m_id;
    >>> unsigned int m_cost;
    >>> bool m_isVisited;
Vertex()
{           
}
for(std::map<int,unsigned int>::iterator iter = m_edgeList.begin();
    iter!=m_edgeList.end(); iter++)

// can be rewritten as:
for(auto& edge: m_edgeList)
RouteCostObject findRout(Graph& g, Vertex& start, Vectex& end)
 {
      OrderedListByDistance    boundryList;
      SetOfVertex              foundVertixes;

      boundryList.push_back([start, 0, []]);

      while(!boundryList.empty())
      {
          // Remove the top items from the list.
          (vertex, cost, route) = boundryList.top();       // Note: boundryList is sorted
          boundryList.pop();

          // If we have reached the end.
          // This is the shortest route.
          if (vertex == end)
          {    return [cost, route];
          }


          // Check if we have already found the shortest distance
          // to this vertex.
          if (foundVertixes.find(vertex) != vertex.end())
          {
              continue;
          }
          foundVertixes.insert(vertex);

          // Add all edges to the boundary list.
          for(auto& edge: vertex.edges)
          {
              boundryList.push_back([edge.dst, cost + edge.cost, route + edge]);
          }
      }
      // No route from start to end
      return [infinity, []];
 }

Context

StackExchange Code Review Q#77225, answer score: 6

Revisions (0)

No revisions yet.