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

Do I need to delete a vector?

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

Problem

I want to make a graph from input from a text file in the format:

vertex1 vertex2 cost time


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:

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 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 const

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

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:

  • edge is not a pointer type, so that assignment won't event compile.



  • p_edge is derferenced though it is never initialized, so that will likely outright-fault due to undefined behavior



  • ifile is 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.