Skip to main content
added 200 characters in body
Source Link
Reinderien
  • 71.2k
  • 5
  • 76
  • 257

Const operators

bool operator==(const Node& node) {

should be

bool operator==(const Node& node) const {

Likewise for long edgeLength(const Node& node1, const Node& node2), edgeExists, printGraph, etc.

Construction

This function:

void createGraph() {
    std::cout << "Enter the number of Nodes: ";
    std::cin >> count;
    for (int i = 0; i < count; i++) {
        std::vector<int> v;
        node_list.push_back(Node(i + 1));
        for (int j = 0; j < count; j++) {
            long temp;
            std::cin >> temp;
            v.push_back(temp);
        }
        matrix.push_back(v);
    }
}

is mostly code that belongs in a constructor. The constructor in this case would accept an istream& and would not cout; that could be done by the caller. The advantage of this approach is that

  1. it is more flexible - you could deserialize from a file, for example;
  2. it is more decoupled.

I realize that createGraph is a private which is called by the existing constructor, which is fine; but I would stop short of baking in cout/cin.

Pointer madness

This:

*(&adj_matrix + 1)

scares mewill not do what you want. You're gettingHave you tried executing this method? Based on the address of adj_matrixlink you gave me, which is nowit seems you were attempting to do a triple pointer; assuminghack that the location after it is valid; and dereferencing it. Canrequires that you have a reference to an array with defined size, but you do not just- you only have bare pointers.

Just pass in an integral matrix size?dimensions.

Boolean expressions

    if (std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) == edge_list.end()) {
        return false;
    }
    return true;

can be

    return std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) != edge_list.end();

Const operators

bool operator==(const Node& node) {

should be

bool operator==(const Node& node) const {

Likewise for long edgeLength(const Node& node1, const Node& node2), edgeExists, printGraph, etc.

Construction

This function:

void createGraph() {
    std::cout << "Enter the number of Nodes: ";
    std::cin >> count;
    for (int i = 0; i < count; i++) {
        std::vector<int> v;
        node_list.push_back(Node(i + 1));
        for (int j = 0; j < count; j++) {
            long temp;
            std::cin >> temp;
            v.push_back(temp);
        }
        matrix.push_back(v);
    }
}

is mostly code that belongs in a constructor. The constructor in this case would accept an istream& and would not cout; that could be done by the caller. The advantage of this approach is that

  1. it is more flexible - you could deserialize from a file, for example;
  2. it is more decoupled.

I realize that createGraph is a private which is called by the existing constructor, which is fine; but I would stop short of baking in cout/cin.

Pointer madness

This:

*(&adj_matrix + 1)

scares me. You're getting the address of adj_matrix, which is now a triple pointer; assuming that the location after it is valid; and dereferencing it. Can you not just pass in an integral matrix size?

Boolean expressions

    if (std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) == edge_list.end()) {
        return false;
    }
    return true;

can be

    return std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) != edge_list.end();

Const operators

bool operator==(const Node& node) {

should be

bool operator==(const Node& node) const {

Likewise for long edgeLength(const Node& node1, const Node& node2), edgeExists, printGraph, etc.

Construction

This function:

void createGraph() {
    std::cout << "Enter the number of Nodes: ";
    std::cin >> count;
    for (int i = 0; i < count; i++) {
        std::vector<int> v;
        node_list.push_back(Node(i + 1));
        for (int j = 0; j < count; j++) {
            long temp;
            std::cin >> temp;
            v.push_back(temp);
        }
        matrix.push_back(v);
    }
}

is mostly code that belongs in a constructor. The constructor in this case would accept an istream& and would not cout; that could be done by the caller. The advantage of this approach is that

  1. it is more flexible - you could deserialize from a file, for example;
  2. it is more decoupled.

I realize that createGraph is a private which is called by the existing constructor, which is fine; but I would stop short of baking in cout/cin.

Pointer madness

This:

*(&adj_matrix + 1)

will not do what you want. Have you tried executing this method? Based on the link you gave me, it seems you were attempting to do a hack that requires that you have a reference to an array with defined size, but you do not - you only have bare pointers.

Just pass in integral matrix dimensions.

Boolean expressions

    if (std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) == edge_list.end()) {
        return false;
    }
    return true;

can be

    return std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) != edge_list.end();
Source Link
Reinderien
  • 71.2k
  • 5
  • 76
  • 257

Const operators

bool operator==(const Node& node) {

should be

bool operator==(const Node& node) const {

Likewise for long edgeLength(const Node& node1, const Node& node2), edgeExists, printGraph, etc.

Construction

This function:

void createGraph() {
    std::cout << "Enter the number of Nodes: ";
    std::cin >> count;
    for (int i = 0; i < count; i++) {
        std::vector<int> v;
        node_list.push_back(Node(i + 1));
        for (int j = 0; j < count; j++) {
            long temp;
            std::cin >> temp;
            v.push_back(temp);
        }
        matrix.push_back(v);
    }
}

is mostly code that belongs in a constructor. The constructor in this case would accept an istream& and would not cout; that could be done by the caller. The advantage of this approach is that

  1. it is more flexible - you could deserialize from a file, for example;
  2. it is more decoupled.

I realize that createGraph is a private which is called by the existing constructor, which is fine; but I would stop short of baking in cout/cin.

Pointer madness

This:

*(&adj_matrix + 1)

scares me. You're getting the address of adj_matrix, which is now a triple pointer; assuming that the location after it is valid; and dereferencing it. Can you not just pass in an integral matrix size?

Boolean expressions

    if (std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) == edge_list.end()) {
        return false;
    }
    return true;

can be

    return std::find(edge_list.begin(), edge_list.end(), Edge(node1, node2)) != edge_list.end();