patterncppMinor
Implementing Directed and Undirected Graph in C++
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
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
-
Pass complex objects by const reference when a copy is not needed. In C++, a function such as
-
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:
Using the assignment 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
-
Use
Rewrite as:
-
Use range based for loops in places like this:
Rewrite as:
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 referenceVertex(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.