Skip to main content
added 13 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

###Is this test not redundant.
while (!q.empty()) { if (visited.find(q.front()) != visited.end()) { q.pop(); continue; }

If you have entered the loop. Then q is not empty. If q is not empty then front() will not be end().

###Is this test not redundant.
while (!q.empty()) { if (visited.find(q.front()) != visited.end()) { q.pop(); continue; }

If you have entered the loop. Then q is not empty. If q is not empty then front() will not be end().

###Std::cout is not the only stream

###Std::cout is not the only stream

###Is this test not redundant.
while (!q.empty()) { if (visited.find(q.front()) != visited.end()) { q.pop(); continue; }

If you have entered the loop. Then q is not empty. If q is not empty then front() will not be end().

###Std::cout is not the only stream

###Is this test not redundant.
while (!q.empty()) { if (visited.find(q.front()) != visited.end()) { q.pop(); continue; }

If you have entered the loop. Then q is not empty. If q is not empty then front() will not be end(). ###Std::cout is not the only stream

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

##General Comments ###Separation of Concerns This principle basically says your class should do either business logic or resource management not both. You break this principle as your Graph object is responsible for maintaining a graph but also has to maintaining the memory (resources) associated with the graph (You try to dynamically manage adjacencyMatrix and distanceMatrix).

It would be better if you split these up into two different classes. Note: Them memory management can be done for you by simply using the std::vector<> to handle the resource management.

###Rule of Three Because you have not separated your concerns your resource management logic is flawed and your code broken (in terms of resource management).

{
    Graph   one(15);
    Graph   two(one);   // Compiler generated copy constructor is not
                        // doing what you actually need.
}
// Resulting in a double delete when these go out of scope.

You should look up the rule of three and implement it.

###Move Semantics. This is 2017. Move semantics have been around since 2011. You should definitely be thinking about how your objects are moved. Currently your classes can only be copied.

You should expand the rule of three to the rule of five on your code.

###Distance calculator is half of dykstra Your distance calculator is basically a simplification of dykstra. I did not read it thouroughly to make sure it works in all situations but I would check out "dykstra Algorithm" you will find many references on the web.

###Visitor Pattern Your DFS and BFS look like they probably work. Though I would look up the visitor pattern. It allows a more generic traversal of the graph and allows arbitrary actions to be taken at each node.

##Code Review

###Don't use this->

new bool*[this->numVertices];

Using this-> hides errors and will result in more bugs. The only reason to actually use this-> is when you have shadowed variables and you want to distinguish between a local and a member.

If you allow shadowed variables you will eventually forget to add this-> and get the wrong one. The compiler will not be able to detect that you got the wrong one and thus you have introduced a bug.

The better solution is to not allow shadowed variables and make the compiler generate an error when you do have shadowed variables. Since you now never have shadowed variables there is never a reason to use this->.

###One Initialize per line Just like normal variable declarations where you should only initialize one variable per line (we are human and we somebody probably has to read your code. You should arrange your initializer list so each member is on a separate line.

Graph::Graph(int inNumVertices): numVertices(MAX(inNumVertices, 0)), distanceMatrixComputed(false) {

This not only increases general readability but quickly allows you to establish order (and verify that it is correct).

Graph::Graph(int inNumVertices)
    : numVertices(MAX(inNumVertices, 0))
    , distanceMatrixComputed(false)
{

###Never do manual memory management

/**
 * Allocate memory for adjacency matrix
 */
void Graph::initAdjacencyMatrix() {
  this->adjacencyMatrix = new bool*[this->numVertices];

  for (int i = 0; i < this->numVertices; ++i) {
    this->adjacencyMatrix[i] = new bool[this->numVertices];
  }
}

Use std::vector they have everything you need built in.

Here you forgot that new can fail. In which case you will end up leaking a lot of memory (if it is not the first new that fails).

###Is this test not redundant.
while (!q.empty()) { if (visited.find(q.front()) != visited.end()) { q.pop(); continue; }

If you have entered the loop. Then q is not empty. If q is not empty then front() will not be end().

###Std::cout is not the only stream

void Graph::printComponents() {

Printing a graph should not change the state of the graph. So you should probably mark it const.

I would pass a stream as a parameter (it can default to std::cout). But the standard idiom for printing is using the operator<<. So you should also define one of those.

void Graph::printComponents(std::ostream& str = std::cout) const;
friend std::ostream& operator<<(std::ostream& str, Graph const& data)
{
    data.printComponents(str);
    return str;
}