Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

I had posted here beforebefore, but I decided that the approach used earlier - where every Vertex stores a vector of pointers to vertices it's adjacent to - was just not worth the hassle (read segfaults) it was causing me. (I'm relatively inexperienced with pointers as of now.)

I had posted here before, but I decided that the approach used earlier - where every Vertex stores a vector of pointers to vertices it's adjacent to - was just not worth the hassle (read segfaults) it was causing me. (I'm relatively inexperienced with pointers as of now.)

I had posted here before, but I decided that the approach used earlier - where every Vertex stores a vector of pointers to vertices it's adjacent to - was just not worth the hassle (read segfaults) it was causing me. (I'm relatively inexperienced with pointers as of now.)

Source Link

Adjacency-list graph implementation in C++ (redone from scratch)

I had posted here before, but I decided that the approach used earlier - where every Vertex stores a vector of pointers to vertices it's adjacent to - was just not worth the hassle (read segfaults) it was causing me. (I'm relatively inexperienced with pointers as of now.)

Besides, a Vertex isn't more than a wrapped long in most of my use cases (implementing basic graph algorithms from CLRS), so passing by value makes sense too. (See end, though.)

Here's the code, please critique this. (I know that the WeightT parameter is just lying there, I'll write the relevant methods later.)

#ifndef _GRAPH_H
#define _GRAPH_H

#include <vector>
#include <memory>
#include <iostream>
#include <unordered_map>
#include <algorithm>
#include <sstream>
#include <fstream>

enum Color { WHITE, GRAY, BLACK, RED, NONE }; //for DFS

//TODO remove this later
//using namespace std;

template <class PayloadT> class Vertex {
    template <class PayloadT_,  //avoids template param shadowing
              class WeightT>    friend class Graph;
    //but is it even required?

    private:
        PayloadT payload;
        Color color;

    public:
        Vertex()            : payload(PayloadT()), color(WHITE)  { }
        Vertex(PayloadT p)  : payload(p), color(WHITE)           { }
        
        Vertex<PayloadT>& operator=(Vertex<PayloadT>& other) {
            swap(*this, other);
            return *this;
        }

        bool operator==(const Vertex<PayloadT>& other) const {
            return (payload == other.get_payload()) && 
                   (color == other.get_color());
        }

        PayloadT get_payload() const     { return payload; }
        PayloadT set_payload(PayloadT p) { payload = p; }

        Color    get_color() const       { return color; }
        Color    set_color(Color c)      { color = c; }
};

//For the std::unordered_map
template <typename PayloadT> struct VertexHasher {
    std::size_t operator()(const Vertex<PayloadT>& v) const {
        return std::hash<PayloadT>()(v.get_payload());
    }
};

template <class PayloadT, class WeightT> class Graph {
    using Vert     = Vertex<PayloadT>;
    //using VertPtr = shared_ptr<Vert>;
    using VertList = std::vector<Vert>;
    using AdjList  = std::unordered_map<Vert, VertList, 
                                   VertexHasher<PayloadT>>;

    private:
        AdjList vertices;

    public:
        Graph()             : vertices()    {}
        Graph(VertList vl)  : vertices()    {
            for (auto i : vl) {
                vertices[i] = VertList();
            }
        }
        Graph(AdjList al)   : vertices(al)  {}

        std::size_t get_size() const { return vertices.size(); }

        void add_vertex(Vert v) { vertices[v] = VertList(); }
        void rm_vertex(Vert v)  { vertices.erase(v); }
        
        VertList get_adjacents(Vert v) const { return vertices[v]; } 
        
        void add_edge(Vert start, Vert end)  { vertices[start].push_back(end); }
        void rm_edge(Vert start, Vert end)   { 
            vertices[start].erase(
                    std::find(vertices.begin(), vertices.end(), end), 
                    vertices.end()); 
        }

        std::string to_gv_dot() const;        
        void display() const;
};

template <class PayloadT, class WeightT>
std::string Graph<PayloadT, WeightT>::to_gv_dot() const {
    std::stringstream stm;
    stm << "digraph {\n";
    for (auto i : vertices) {
        for (auto j : i.second) {
            stm << "    " <<  i.first.get_payload() << " -> "
                << j.get_payload() << ";\n";
        }
    }
    stm << "}\n";
    return stm.str();
}

//this is bad and hacky and everything
template <class PayloadT, class WeightT>
void Graph<PayloadT, WeightT>::display() const {
    std::string repr = this->to_gv_dot();
    static std::ofstream out("/tmp/out.gv");
    out << repr << std::endl;
    system("dot -Tpng /tmp/out.gv -o /tmp/out.png");
    system("eog /tmp/out.png");
    system("rm -f /tmp/out.png");
}

#endif //_GRAPH_H

Really, though, I'd prefer the pointer-based approach, as it'd allow me to use this for larger payloads (I think even a Graph<string> would show some performance improvements.)

I'd like some pointers (NPI) on how I could convert this code to make it store pointer vectors instead.

Also, is there any downside to putting the smaller functions directly in the class body, apart from readability?

(As for why I decided to start from scratch when I prefer the other approach to this . . . I'd gotten tired of the other code and simply wanted to start anew.)