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

Implementing Directed and Undirected Graph in C++

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

Problem

I am learning C++ and I decided to implement a Directed and UnDirected graph. (I haven't learned how to do inheritance yet, so they are distinct classes.) I have not handled any error cases (i.e. trying to remove a vertex that doesn't exist.)

I am interested in any comments/criticisms of my approach.

```
#include
#include
#include
#include
#include

using namespace std;

class Graph;
class Edge;
class Vertex;

class Edge {
int weight;
Vertex * vertex1;
Vertex * vertex2;
public:
int getWeight() const {return weight;}
Vertex* getV1() const {return vertex1;}
Vertex* getV2() const {return vertex2;}
void setWeight(int w){weight=w;}
void setV1(Vertex * v){vertex1=v;}
void setV2(Vertex * v){vertex2=v;}
Edge(int w, Vertex v1, Vertex v2){weight=w;vertex1=v1;vertex2=v2;}
};

class Vertex {
string label;
vector edgesLeavingMe;
bool visited;
public:
string getLabel() const {return label;}

vector getEdges()const{return edgesLeavingMe;}

Edge * getEdgeTo(string d){
for (vector::iterator it = edgesLeavingMe.begin(); it != edgesLeavingMe.end(); ++it){
if ((*it)->getV2()->getLabel()==d){
return (*it);
}
}
return 0;
}
void setVisited(bool v){visited=v;}
bool getVisited() {return visited;}
void addEdge(Edge * e){edgesLeavingMe.push_back(e);}
void removeEdge(Edge * e){edgesLeavingMe.erase(remove(edgesLeavingMe.begin(),edgesLeavingMe.end(),e),edgesLeavingMe.end());}

void removeEdgeTo(string l){
Edge * e = getEdgeTo(l);
removeEdge(e);
}

Vertex(string l){label=l; visited=false;}
};

class Graph {
vector edges;
map vertices;
public:
Vertex * addVertex(string label){
Vertex * v = new Vertex(label);
vertices[label]=v;
return v;
}
Edge * addEdge(int w, string from, string to);
void removeEdge(string from, string to);
Vertex * getVertexWithlabel(string l);
void removeVertex(string l);
};

class UnDirectedGraph {
vector edges;
map vertices;
public:
Vertex * addVert

Solution

-
You should avoid using namespace, specially in a header file. Doing so exposes all of the contents of a namespace to the global scope. That defeats the purpose of a namespace, which is allowing for potentially colliding names to coexist. Read this question for more details.

-
Pass complex objects by const reference when a copy is not needed. In C++, a function such as void foo(std::string bar); takes bar "by value", meaning that the bar string is copied into the function. The default in C++ is always a copy, even for object instances. When your methods/functions are only inspecting a parameter and that parameter is not a native type like int, float, etc, you should pass the parameter by const reference to avoid an unnecessary copy. E.g.:

void foo(const std::string & bar) { /* ... */ }
//       ^^^^^             ^
//       const         reference


-
Make sure to initialize data in constructors using the constructor initialization list. This will ensure that you initialize your member objects only once, by calling their own constructors:

Vertex(string l)
    : label(l)
    , visited(false)
{ }


Using the assignment operator (=) inside the constructor body might result in unnecessary work, since first the default constructor for the object will be called, then you'll init again with operator =.

-
Be more consistent and careful with indentation. I can spot several places where your are missing tabs or have uneven spacing. That makes your code look untidy. Also consider inserting a few blank lines to separate unrelated things. Your code is a bit too crammed for my eyes.

-
If you have access to C++11, there are a few nice improvements an cleanups you can do with it:

-
Use the nullptr literal to replace the uncanny 0 and the old & ugly NULL.

-
Use auto for type inference. It can greatly simplify long statements like this:

vector edges = v->getEdges();


Rewrite as:

auto edges = v->getEdges();


-
Use range based for loops in places like this:

for (vector::const_iterator it = edges.begin(); it != edges.end(); ++it) { /* ... */ }


Rewrite as:

for (auto & edge : edges) { /* ... */ }

Code Snippets

void foo(const std::string & bar) { /* ... */ }
//       ^^^^^             ^
//       const         reference
Vertex(string l)
    : label(l)
    , visited(false)
{ }
vector<Edge *> edges = v->getEdges();
auto edges = v->getEdges();
for (vector<Edge *>::const_iterator it = edges.begin(); it != edges.end(); ++it) { /* ... */ }

Context

StackExchange Code Review Q#87892, answer score: 3

Revisions (0)

No revisions yet.