patterncppModerate
depthFirstTraverse fully in C++?
Viewed 0 times
depthfirsttraversefullystackoverflow
Problem
Wondering if this is fully in C++, and if not can someone help me tell the differences. This was submitted by me last semester, and received a good grade. I'm currently trying to ensure I can tell C++ and C apart.
#include
#include
#include //Not neccessary in C++
#define swap(type, a, b) {type t = a; a = b; b = t;}
using namespace std;
typedef struct {
int degree, *adjList, //arraySize = degree
nextToVisit, //0 stackArray;
for (int i = 0; i %d\n", stackArray.top()); }
else { printf("backtracking %d -> %d\n", stackArray.top(), startNode);
stackArray.pop(); }
}
else { nextNode = nodes[stackArray.top()].adjList[nextToVisit[stackArray.top()]];
nextToVisit[stackArray.top()]++;
if (0 == dfLabels[nextNode]) {
parents[nextNode] = stackArray.top();
printf("processing (%d, %d): tree-edge, dfLabel(%d) = %d\n",
stackArray.top(), nextNode, nextNode, lastDFLabel+1);
stackArray.push(nextNode); }
else if (dfLabels[nextNode] > numNodes;
nodes = new GraphNode[numNodes];
for (int i = 0; i > nodes[i].nodeName >> ignore >> nodes[i].degree >> ignore;
nodes[i].adjList = new int[nodes[i].degree];
for (int j = 0; j > nodes[i].adjList[j];
} digraph.close();
printf ("Input Graph:\nnumNodes = %d\n", numNodes);
for (int i = 0; i< numNodes; i++) {
printf("%d (%d): ", nodes[i].nodeName, nodes[i].degree);
for (int j = 0; j < nodes[i].degree; j++)
for (int k = 0; k < j; k++)
if (nodes[i].adjList[j] < nodes[i].adjList[k])
swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);
for (int j = 0; j < nodes[i].degree; j++) printf("%d ", nodes[i].adjList[j]);
printf("\n");
} for (int i = 0; i < 30; i++) printf("-");
printf ("\n");
}Solution
It's still closer to C than C++ IMO.
First of two non C++ issues:
-
You don't encapsulate your classes. Your whole interface is public. Thus you are now doomed to forever maintain this interface for eternity. Additionally it allows other people to accidentally modify your structure. Use the C++ class system to encapsulate your objects and at least protect your objects from accidental miss use.
-
Your tree-traversal actually depends on the node object to track traversal. Now that's a bad design thing. You are basically binding yourself to an implementation and limiting the usability of your code. You should most defiantly look up the
The rest are C++ issues:
You are doing resource management manually:
It would be better to use a std::vector at least that would be exception safe in controlling the resource:
You are manually loading nodes. Should a node not know how to stream itself onto and off a stream?
I would expect it to look like this:
Or even better create items and push them into the vector:
Since the only thing that seems to be in the file "digraph.data" seem to be GraphNodes then you could do something like this:
This is not C++
The problem with the printf style is that it is not type safe. The main advantage C++ has over C is the exceptional type safety of the language. Use this to your advantage.
This is so bad I nearly swallowed my tounge.:
You have #defined swap.
The reason is to make sure macros do not clash with any other identifiers.
If there was a conversion from this object to int the compiler will now happily do it.
If you want to override the default swap behavior you just need to rewrite your type specific version and Koenig lookup will automatically find it.
Not a big deal but prefer pre-increment. The reason is it does not matter for POD types but for user defined types it does (or potentially does) matter. If a maintainer at a latter stage changes the type of the loop variable then you don't have to go and change any of the code the pre-increment is already the correct type.
Don't declare multiple variables on the same line:
Every coding standard is going to tell you not to do it. So get used to it. Technically there are a few corner cases that will come and hit you and putting each variable on its own line makes it easier to read. Also note it looks like you are initializing all values to zero. That's not happening so all of these (apart from the last one) are undefined.
Also defining arrays with a non cost type is not technically allowed.
Some compilers allow it because it is C90 extension. But its not really portable. Use std::vector (or std::array) instead.
Don't put code on the same line as the close braces.
I dobt anybody is going to like that style even the people that want to save vertical line space.
And I prefer to put the statement the for loop is going to run on the next line so it is obvious what is happening (but I am not going to care that much about it).
typedefing a structure is not needed in C++
This is just
When you loop over your containers you are using a C style (ie you are using an index). It is usually more conventionally to use an iterator style to index over a container (even a simple array). Thus if you ever choose to move to another style of container the code itself does not need to be changed. The iterator style makes the code container agnostic.
Personally I don't think it is a big deal. In this limited context.
You implement (badly) a sort function:
Lets just say in both C/C++ you should probably use the built-in sorting algorithms:
```
std::sort(&nodes[i].adjList[0], &nodes[i].adjList[nodes[i].de
First of two non C++ issues:
-
You don't encapsulate your classes. Your whole interface is public. Thus you are now doomed to forever maintain this interface for eternity. Additionally it allows other people to accidentally modify your structure. Use the C++ class system to encapsulate your objects and at least protect your objects from accidental miss use.
-
Your tree-traversal actually depends on the node object to track traversal. Now that's a bad design thing. You are basically binding yourself to an implementation and limiting the usability of your code. You should most defiantly look up the
Visitor Pattern.The rest are C++ issues:
You are doing resource management manually:
nodes = new GraphNode[numNodes];
// STUFF
nodes[i].adjList = new int[nodes[i].degree];It would be better to use a std::vector at least that would be exception safe in controlling the resource:
std::vector nodes(numNodes);You are manually loading nodes. Should a node not know how to stream itself onto and off a stream?
digraph >> nodes[i].nodeName >> ignore >> nodes[i].degree >> ignore;I would expect it to look like this:
digraph >> nodes[i];Or even better create items and push them into the vector:
Since the only thing that seems to be in the file "digraph.data" seem to be GraphNodes then you could do something like this:
std::copy(std::istream_iterator(digraph),
std::istream_iterator(),
std::back_inserter(nodes) // Assuming nodes is a vector described above.
);This is not C++
printf("%d (%d): ", nodes[i].nodeName, nodes[i].degree);The problem with the printf style is that it is not type safe. The main advantage C++ has over C is the exceptional type safety of the language. Use this to your advantage.
This is so bad I nearly swallowed my tounge.:
swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);You have #defined swap.
- Use all caps for macros.
The reason is to make sure macros do not clash with any other identifiers.
- Macros are not type safe.
If there was a conversion from this object to int the compiler will now happily do it.
- There is already a type safe macro as part of the standard library.
If you want to override the default swap behavior you just need to rewrite your type specific version and Koenig lookup will automatically find it.
Not a big deal but prefer pre-increment. The reason is it does not matter for POD types but for user defined types it does (or potentially does) matter. If a maintainer at a latter stage changes the type of the loop variable then you don't have to go and change any of the code the pre-increment is already the correct type.
for (int i = 0; i < 30; ++i)
// ^^^^ Prefer pre-increment hereDon't declare multiple variables on the same line:
int nextNode, dfLabels[numNodes], nextToVisit[numNodes], parents[numNodes], lastDFLabel = 0;Every coding standard is going to tell you not to do it. So get used to it. Technically there are a few corner cases that will come and hit you and putting each variable on its own line makes it easier to read. Also note it looks like you are initializing all values to zero. That's not happening so all of these (apart from the last one) are undefined.
Also defining arrays with a non cost type is not technically allowed.
dfLabels[numNodes],Some compilers allow it because it is C90 extension. But its not really portable. Use std::vector (or std::array) instead.
Don't put code on the same line as the close braces.
I dobt anybody is going to like that style even the people that want to save vertical line space.
} for (int i = 0; i < 30; i++) printf("-");And I prefer to put the statement the for loop is going to run on the next line so it is obvious what is happening (but I am not going to care that much about it).
typedefing a structure is not needed in C++
typedef struct {
// BLAH
} GraphNode;This is just
struct GraphNode
{
// BLAH
};When you loop over your containers you are using a C style (ie you are using an index). It is usually more conventionally to use an iterator style to index over a container (even a simple array). Thus if you ever choose to move to another style of container the code itself does not need to be changed. The iterator style makes the code container agnostic.
Personally I don't think it is a big deal. In this limited context.
for (int j = 0; j < nodes[i].degree; j++)You implement (badly) a sort function:
for (int j = 0; j < nodes[i].degree; j++)
for (int k = 0; k < j; k++)
if (nodes[i].adjList[j] < nodes[i].adjList[k])
swap(int, nodes[i].adjList[j], nodes[i].adjList[k]);Lets just say in both C/C++ you should probably use the built-in sorting algorithms:
```
std::sort(&nodes[i].adjList[0], &nodes[i].adjList[nodes[i].de
Code Snippets
nodes = new GraphNode[numNodes];
// STUFF
nodes[i].adjList = new int[nodes[i].degree];std::vector<GraphNode> nodes(numNodes);digraph >> nodes[i].nodeName >> ignore >> nodes[i].degree >> ignore;digraph >> nodes[i];std::copy(std::istream_iterator<GraphNode>(digraph),
std::istream_iterator<GraphNode>(),
std::back_inserter(nodes) // Assuming nodes is a vector described above.
);Context
StackExchange Code Review Q#4095, answer score: 13
Revisions (0)
No revisions yet.