patterncppMinor
Do I need to delete a vector?
Viewed 0 times
vectordeleteneed
Problem
I want to make a graph from input from a text file in the format:
where vertex1 and vertex2 are strings that form an edge and cost and time are the properties of that edge. This information will be used to construct the Graph based on cost. I want to plan out the project before coding it so I have a good idea of what to do. So far I decided on the following classes:
Is my design correct or is it lacking something? Can it be improved?
vertex1 vertex2 cost timewhere vertex1 and vertex2 are strings that form an edge and cost and time are the properties of that edge. This information will be used to construct the Graph based on cost. I want to plan out the project before coding it so I have a good idea of what to do. So far I decided on the following classes:
class Edge {
private:
int vertex1;
int vertex1;
int cost;
int time;
public:
Edge(int v1, int v2, int cost, int time) {
this->vertex1 = v1;
this->vertex2 = v2;
this->cost = cost;
this->time = time;
}
~Edge();
int getEdgeCost();
int getEdgeTime();
};
class Graph {
private:
vector edge; //edges in graph
public:
Graph() : edge(0) {}
~Graph(){}
int getTotalCost();
int getTotalTime();
void addEdge(Edge e);
Graph getMinSpanTree();
};
class Vertex {
private:
string name;
int number;
bool visited;
vector connEdges; // vector of all edges connected to this vertex
public:
Vertex(string name, int number) {
this->name = name;
this->number = number;
visited = true;
}
~Vertex();
string getName();
int getNum();
bool wasVisited();
Graph getShortestPath();
};
int main(int argc, char* argv[]) {
std::string ifileName = std::string(argv[1]);
ifstream ifile;
if(ifile)
ifile.open(ifileName);
Edge edge;
Graph* p_graph;
Vertex v;
string vert1, vert2;
int cost, time;
int i = 1; // numbers each vertex
while(ifile.good()) {
// read in input, put it in vars above, skipped for brevity
// make the vertices
v1 = new Vertex(vert1, i);
i++; // increments vertex number
v2 = new Vertex(vert2, i);
i++;
// make the edge
edge = new Edge(v1.getNum(), v2.getNum(), cost, time);
// add it to graph
p_graph->addEdge(edge);
}Is my design correct or is it lacking something? Can it be improved?
Solution
Overall
Resolve all standard library entities with
Class Edge
You use initializer lists in some places (
Class Graph
There are outright compilation bugs in this class. There is no such entity as
Class Vertex
As previously mentioned, use initializer lists and full
main()
This function is simply loaded with errors. Among them:
Resolve all standard library entities with
std::. Never put using namespace declarations in a header file, where most of his belongs.Class Edge
You use initializer lists in some places (
Graph), not in others. Be consistent. I advise using them always where they're needed, and not at all where they're not. Also, there is no need for a destructor in this class unless you're expecting polymorphism and need virtual-dtor for a vtable. Finally, make your getters constclass Edge {
private:
int vertex1;
int vertex2;
int cost;
int time;
public:
Edge(int v1, int v2, int cost, int time)
: vertex1(v1), vertex2(v2), cost(cost), time(time)
{
}
int getVertex1() const { return vertex1; }
// etc...
};Class Graph
There are outright compilation bugs in this class. There is no such entity as
Edges, yet you have a vector of them. Further, properly named it should be edges are a vector of Edge. Also, the initializer list in this class is pointless, as is the destructor and, as-written, the constructor. Finally, you may (or may not) want to pass your incoming edge as a const-ref (lots of opinions on that, I know).class Graph {
private:
vector edges; //edges in graph
public:
int getTotalCost();
int getTotalTime();
void addEdge(const Edge& e);
Graph getMST(); // get Minimum Spanning Tree
};Class Vertex
As previously mentioned, use initializer lists and full
std:: namespace resolution. Like Graph, this uses Edges as a typename, but no such type exists. Again, we use const qualifiers on all getters, here also returning the name by const-ref rather than value-copy. Finally, no chance of ever setting visited to true unless you provide a setter for it, nor accessing or modifying the edges vector without providing some means to do so.class Vertex
{
private:
std::string name;
int number;
bool visited;
std::vector connEdges; // vector of all edges connected to this vertex
public:
Vertex(const std::string& name, int number)
: name(name), number(number), visited(false)
{
}
// edge accessors
void addEdge(const Edge& edge) { connEdges.push_back(edge); }
const std::vector& getEdges() const { return edges; }
// properties
const std::string& getName() const { return name; }
int getNum() const { return number; }
bool wasVisited() const { return visited; }
void setVisited(bool val = true) { visited = val; }
Graph getShortestPath();
};main()
This function is simply loaded with errors. Among them:
edgeis not a pointer type, so that assignment won't event compile.
p_edgeis derferenced though it is never initialized, so that will likely outright-fault due to undefined behavior
ifileis never opened before use, so nothing will be read there.
- it leaks memory, but only because of an error previously mentioned above. Were that coded correctly this would be a non-issue.
Code Snippets
class Edge {
private:
int vertex1;
int vertex2;
int cost;
int time;
public:
Edge(int v1, int v2, int cost, int time)
: vertex1(v1), vertex2(v2), cost(cost), time(time)
{
}
int getVertex1() const { return vertex1; }
// etc...
};class Graph {
private:
vector<Edge> edges; //edges in graph
public:
int getTotalCost();
int getTotalTime();
void addEdge(const Edge& e);
Graph getMST(); // get Minimum Spanning Tree
};class Vertex
{
private:
std::string name;
int number;
bool visited;
std::vector<Edge> connEdges; // vector of all edges connected to this vertex
public:
Vertex(const std::string& name, int number)
: name(name), number(number), visited(false)
{
}
// edge accessors
void addEdge(const Edge& edge) { connEdges.push_back(edge); }
const std::vector<Edge>& getEdges() const { return edges; }
// properties
const std::string& getName() const { return name; }
int getNum() const { return number; }
bool wasVisited() const { return visited; }
void setVisited(bool val = true) { visited = val; }
Graph getShortestPath();
};Context
StackExchange Code Review Q#30609, answer score: 7
Revisions (0)
No revisions yet.