patterncppMinor
A* versus Bidirectional Dijkstra's algorithm
Viewed 0 times
algorithmbidirectionaldijkstraversus
Problem
I have added bidirectional Dijkstra's algorithm into my pathfinding "framework", and I would like to make good use of C++ programming idioms, eliminate all possible memory leaks, otherwise improve readability, but I need your help for that to happen.
That's what I have scrambled:
shortest_path.h:
```
#ifndef SHORTEST_PATH_H
#define SHORTEST_PATH_H
#include
#include
#include
#include
#include
namespace coderodde {
template
class AbstractGraphNode {
protected:
using Set = std::unordered_set;
public:
AbstractGraphNode(std::string name) : m_name{name} {}
virtual void connect_to(NodeType* other) = 0;
virtual bool is_connected_to(NodeType* other) const = 0;
virtual void disconnect_from(NodeType* other) = 0;
virtual typename Set::iterator begin() const = 0;
virtual typename Set::iterator end() const = 0;
class ParentIterator {
public:
ParentIterator() : mp_set{nullptr} {}
typename Set::iterator begin()
{
return mp_set->begin();
}
typename Set::iterator end()
{
return mp_set->end();
}
void set_list(Set* p_list)
{
this->mp_set = p_list;
}
private:
std::unordered_set* mp_set;
};
virtual ParentIterator* parents() = 0;
bool operator==(const NodeType& other) const
{
return m_name == other.m_name;
}
std::string& get_name() {return m_name;}
protected:
std::string m_name;
};
template
class AbstractWeightFunction {
public:
virtual FloatType& operator()(T p_node1, T p_node2) = 0;
};
template
class Point3D {
private:
const FloatType m_x;
const FloatType m_y;
const FloatType m_z;
public:
Point3D(const FloatType x = FloatType(),
That's what I have scrambled:
shortest_path.h:
```
#ifndef SHORTEST_PATH_H
#define SHORTEST_PATH_H
#include
#include
#include
#include
#include
namespace coderodde {
template
class AbstractGraphNode {
protected:
using Set = std::unordered_set;
public:
AbstractGraphNode(std::string name) : m_name{name} {}
virtual void connect_to(NodeType* other) = 0;
virtual bool is_connected_to(NodeType* other) const = 0;
virtual void disconnect_from(NodeType* other) = 0;
virtual typename Set::iterator begin() const = 0;
virtual typename Set::iterator end() const = 0;
class ParentIterator {
public:
ParentIterator() : mp_set{nullptr} {}
typename Set::iterator begin()
{
return mp_set->begin();
}
typename Set::iterator end()
{
return mp_set->end();
}
void set_list(Set* p_list)
{
this->mp_set = p_list;
}
private:
std::unordered_set* mp_set;
};
virtual ParentIterator* parents() = 0;
bool operator==(const NodeType& other) const
{
return m_name == other.m_name;
}
std::string& get_name() {return m_name;}
protected:
std::string m_name;
};
template
class AbstractWeightFunction {
public:
virtual FloatType& operator()(T p_node1, T p_node2) = 0;
};
template
class Point3D {
private:
const FloatType m_x;
const FloatType m_y;
const FloatType m_z;
public:
Point3D(const FloatType x = FloatType(),
Solution
You have posted a lot of code, which makes it hard (for me) to find structural issues. However a few style items directly caught my attention:
- All your code seems to be in a single header file. When you write libraries (or a framework as you indicate) you want to expose your end user to as little details as possible. Consider splitting logic to a C++ implementation and header file.
- I see a lot of functions accepting pointers. Consider using references, e.g., with
is_valid_path- by using references you can reduce the ASCII art in this line:!(p_path)[i]->is_connected_to((p_path)[i + 1]
- The function
create_random_graphtakesfloatsanddoubles, perhaps a simplified interface with onlydoublesmakes usage simpler.
- There are a lot of
newstatements. Especially with trivial functions such ascreate_random_pointconsider returning by value or using one of the smart pointers from the `header.
- There are no code comments. For example: When you compare arc_load_factor
I can only guess why you used0.9. Similarly vague issues arise when you cast floats to size_t.
- I cannot vouch for your implementation of std::heap
, but the standard does not have a very strict performance upper-bound. For example: a fibonacci heap hasO(1)on operators in which the C++ standard requires onlyO(log n)
- Highly personal, but I'd prefer std::function
over functors. Especially whenoperator()is not declaredconst. Theoretically, your operator could maintain an internal state.
- The
header has all kinds of type-safe features and operators. With yourget_millisecondsfunctions you reduce all the glory to aunsigned long long. Just usestd::chrono::time_point`, it has 'difference' operators defined.
- I find it confusing to see some variable names completely in uppercase.
Context
StackExchange Code Review Q#87872, answer score: 3
Revisions (0)
No revisions yet.