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

A* versus Bidirectional Dijkstra's algorithm

Submitted by: @import:stackexchange-codereview··
0
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(),

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_graph takes floats and doubles, perhaps a simplified interface with only doubles makes usage simpler.



  • There are a lot of new statements. Especially with trivial functions such as create_random_point consider 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 used 0.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 has O(1) on operators in which the C++ standard requires only O(log n)



  • Highly personal, but I'd prefer std::function over functors. Especially when operator() is not declared const. Theoretically, your operator could maintain an internal state.



  • The header has all kinds of type-safe features and operators. With your get_milliseconds functions you reduce all the glory to a unsigned long long. Just use std::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.