0

After extensive testing and debugging, I cannot for the life of my find out why my topological sort algorithm produces the incorrect output. It simply lists the values of the nodes in descending order instead of sorting them topologically. I have listed all relevant classes/input files. Any hints or help is appreciated, thanks in advance. Header for class graph:

/*
2/19/2016
This is the header for class graph.  It includes the definition of a node
and the function signatures
*/
#pragma once

#include <iostream>

using namespace std;

struct node
{
    // actual value at each node
    int value;
    // discovered time
    int d;
    // finished time
    int f;
    // keep track of how many edges each vertex has
    int numEdges;
    // keep track of color of node
    char color;
    // parent (previous) node
    node* p;
    // next node
    node* next;
};


// Class to represent a graph
class Graph
{
public:
    // constructor, give number of vertexes
    Graph(int V);

    // depth first search
    void DFS();

    // function to print sorted nodes
    void print();

    // function for reading file into adjacency list
    void readFile(istream& in);

private:
    // private function called in depth first search, visits every vertex
    // of each edge in the graph
    void DFSVisit(node* u);

    // number of vertices
    int V;

    // array of node pointers, first node in each array is
    // the vertex and following nodes are edges
    node* adj[9];

    // linked list to keep track of the sorted list found from depth first search
    node* sorted;

    // keep track of when each node is discovered/finished
    int time;

    // keep track of number of backedges
    int backEdge;
};

The cpp file for class graph

/*
2/19/2016
This is the cpp file for class graph.  It defines function behavior
*/

#include "Graph.h"

using namespace std;

#include <iostream>
#include <string>

Graph::Graph(int V)
{
    // set graph's number of vertexes to number input
    this->V = V;
    this->backEdge = 0;
}


// Depth first search
void Graph::DFS()
{
    // initialize all colors to white and parent to null
    for (int i = 0; i < V; i++)
    {
        adj[i]->color = 'w';
        adj[i]->p = NULL;
    }
    // initialize time to 0
    time = 0;
    // for each vertex, if it is white, visit its adjacent nodes
    for (int i = 0; i < V; i++)
    {
        if (adj[i]->color == 'w') {
            DFSVisit(adj[i]);
        }
    }
}

// Visit node used by depth first search
void Graph::DFSVisit(node* u)
{
    // increment time
    time++;
    // set u's discovered time
    u->d = time;
    // set color to grey for visited but not finished
    u->color = 'g';
    // visit each adjacency, number of adjacencies stored by numEdges
    for (int i = 0; i < u->numEdges; i++)
    {
        // create node pointing at u next
        node* v = u->next;
        // if the node is already grey, then it is a backedge
        if (v->color == 'g') {
            backEdge++;
        }
        // if it is white and undiscovered, set its parent to u and visit v's next nodes
        else if (v->color == 'w') {
            v->p = u;
            DFSVisit(v);
        }
    }
    // set last node to black
    u->color = 'b';
    // increment time
    time++;
    // set finishing time
    u->f = time;

    if (backEdge == 0) {
        // adds a node to front of linked list that contains sorted values
        node* newNode = new node;
        newNode->next = sorted;
        newNode->value = u->value;
        sorted = newNode;
    }
}

void Graph::print()
{
    if (backEdge == 0) {
        node* curr = sorted;
        if (sorted == NULL) {
            return;
        }
        else {
            cout << "Sorted List:\n";
            for (; curr; curr = curr->next)
            {
                cout << curr->value << " ";
            }
            cout << endl;
        }
    }
    else cout << "Backedges: " << backEdge << endl;
}

void Graph::readFile(istream& in)
{
    // create node pointers to use later
    node* head;
    node* prev;
    node* curr;

    // temp string to use while reading file
    string temp;
    int j;
    // loop iterate vertex number of times
    for (int i = 0; i < V; i++)
    {
        // 3rd character in string holds name of first edge
        j = 3;
        // read line by line
        getline(in, temp);

        // debug print out adjacency list
        // cout << temp << endl;

        // create head node, set value to value of vertex, put it at beginning of each linked list
        head = new node;
        head->value = i + 1;
        adj[i] = head;
        // set numEdges to 0 when row is started
        adj[i]->numEdges = 0;
        // set prev to head at end of each outer loop
        prev = head;

        // read every adjacency for each vertex, once j goes outside of string reading is done
        while (j < temp.length()) {
            // increment numEdges, meaning vertex has one more adjacency
            adj[i]->numEdges++;
            // create node and put in value, found by moving j up two spaces and subtracting 48
            // because it is a char casted as an int
            curr = new node;
            curr->value = (int)temp.at(j) - 48;
            // connect node, increment j by 2 because adjacencies separated by a whitespace
            prev->next = curr;
            prev = curr;
            j += 2;
        }
    }
}

The driver for the program

/*
2/19/2016
This is the driver for the topological sort project.  It reads a file of
vertexes and edges into an adjacency list and performs a depth first
search on that graph representation, creating a topological sort
if no backedges exist, this indicates a DAG or directed acyclic graph
if backedges do exist, this indicates a graph containing cycles meaning
it cannot be topologically sorted
*/

#include <iostream>
#include <fstream>
#include <string>
#include "Graph.h"

using namespace std;

string FILE_NAME = "graphin-DAG.txt";
int NUM_VERTICES = 9;

int main()
{
    // create graph object giving number of vertices
    Graph myGraph(NUM_VERTICES);

    // open file
    ifstream fin(FILE_NAME);

    // validate that file was successfully opened, without file print
    // error and exit program
    if (!fin.is_open()) {
        cerr << "Error opening " + FILE_NAME + " for reading." << endl;
        exit(1);
    }

    // read file into adjacency list
    myGraph.readFile(fin);

    // perform depth first search
    myGraph.DFS();

    // if graph is a DAG, print topological sort, else print backedges
    // this is handled by the print function checking backedges data member
    myGraph.print();
}

And the input file

1: 2
2: 3 8
3: 4
4: 5
5: 9
6: 4 7
7: 3 8
8: 9
9:

Also a visual representation of the graph represented by the adjacency list: https://i.stack.imgur.com/oCImv.png

  • 1
    When you used your debugger, what did you find out? Also, you should test a smaller sample so that you can follow your code (while debugging) to see where it goes wrong. If you can't sort 3 or 4 items, it makes no sense trying to sort 9 of them. – PaulMcKenzie Feb 25 '16 at 19:09
  • Off topic: If you must use `using namespace std;`, [and you probably shouldn't](http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice), do it after including the headers so you don't risk blowing up stuff inside the headers that expects the std namespace to be where it belongs. – user4581301 Feb 25 '16 at 19:23
  • How are you representing a node having multiple children? e.g. `7` in your picture. – Barry Feb 25 '16 at 19:29
  • I am using an adjacency list to represent a node that has multiple children. For example, node 7's next node pointer would be 3, then 3's next node pointer would be 8 - @Barry – CrazieJester Feb 25 '16 at 20:53
  • Thanks for the tip user4581301, I will keep that in mind. – CrazieJester Feb 25 '16 at 20:54
  • @CrazieJester Not sure you ever actually visit *all* the children. – Barry Feb 25 '16 at 20:56
  • PaulMcKenzie, using the debugger it would seem that in the DFSVisit function the else if (v->color == 'w') block never gets executed, but I am not sure how to fix this. Also, I tried it with a smaller file and it works the same, to where it simply puts the nodes in descending order. – CrazieJester Feb 25 '16 at 20:58
  • @Barry Hmm, what makes you say that? What I did was make it so that each head node in the adjacency list tracks how many edges it has and then the for loop in DFSVisit executes a number of times based on that number, shouldn't it visit each edge then? Or why not? – CrazieJester Feb 25 '16 at 21:05
  • `then 3's next node pointer would be 8` : but 3's next in the graph is 4. You need an adjacency list per node. – Thomas Feb 25 '16 at 22:27
  • @Thomas yeah I just meant that for that index of the array of node heads, if you'll look in the graph header I have adj[9] which holds the head node for each adjacency list so it is set up that way. – CrazieJester Feb 25 '16 at 23:14

1 Answers1

0

I think the main problem was that there was some confusion between the 'real' nodes and the nodes in your adjacency list. At least I got confused, so I split the structure into struct Node and struct Adj. The graph now has a Node* nodes[9] for the nodes.

struct Node;

struct Adj
{
    Node*  node;
    Adj*   next;
};


struct Node
{
    // actual value at each node
    int value;
    // discovered time
    int d;
    // finished time
    int f;
    // keep track of color of node
    char color;
    // the list od adjacencies for the node
    Adj*  adj;
};

and things almost instantly seem to work. The answer

Sorted List:
6 7 3 4 5 1 2 8 9

seems correct, [6 7 3 4 5] and [1 2 8 9]. See working example here

Please note that there are still numerous issues with the code, esp. with regard to memory management. Consider using a vector<Node> and a std::vector<Adj>. There are also uninitialized variables in the structs.

Thomas
  • 4,980
  • 2
  • 15
  • 30
  • Thank you a ton! You made me realize the problem. I was silly and created entirely new nodes to point at in the adjacency list where I should have been pointing at the actual nodes themselves. I altered my code to initialize the nodes in the constructor and then point at the node in the adjacency list specified by index instead of value. Thanks again I really appreciated your help! – CrazieJester Feb 26 '16 at 01:45
  • For fun I pimpled your code some more. If you are interested, you can look [here](http://coliru.stacked-crooked.com/a/8a57bbe87ed09fb1). The next move might be to move `dfs` out of graph and give graph a `node` method to enable that. – Thomas Feb 26 '16 at 18:02