Feedback Please

Boston

Doing a a small assignment. Programming element is grand, expect this is the first time I'm going to have to include comments for other people in my program. basically I was wonder if some of you guys could read the rpogram and give me feedback on weather or not you can follow what it does, based on the the comments. o and hte object of the program is to find a route through a mze. The input data is in the forum, width,height, start node, target node, and then a list of connected nodes.



[PHP] /*
Name: Maze
Copyright:
Author:
Date: 29/11/04
Description: Path finder program.

last edit: 11/12/04
*/


#include<iostream>
#include<fstream>
#include<cmath>
using namespace std;

//This, I will use to store values in a LIFO order;
class stack {

private:
int contents[500]; //Holds the values of the stack;
int top; //Indicates the top postion in the stack;
bool full(); //Tests if a stack is full;
bool empty(); //Tests if a stack is empty;

public:
stack(); //constructor
void push(int); //Adds a value to stack;
int pop(); //Removes and returns the top value;
void printstack(ostream&); //prints stack;
};

//Stack contructor;
stack::stack(){
for(int i =0; i <500; i++){
contents =0;

top = 0;
}
}

//Test function;
bool stack::full(){
if(top == 500)
return true;
else
return false;

}

//Test function;
bool stack::empty(){
if(top == 0)
return true;
else
return false;
}

//add fuction;
void stack::push(int a){
if(full()){
cout<<"Stack overflow ";
system("pause");
} else{
contents[top] = a;
top = top +1 ;
}

}

//remove fuction;
int stack::pop(){
if(empty()){

cout<<"Stack underflow";
system("pause");
} else{
top = top-1;
return contents[top];
}

}

//print stack fucntion;
void stack::printstack(ostream& out){
for(int i =0; i <top; i++){
out<< contents<<" ";
}

out<<endl;

}

//This is my adjacency class; it will be my means for
//storing nodes position relative to each other;
class adylist{

public:
int max; //Mazimum node value;
adylist(int,int); //set matrix to zero;
int nodearray[500][500]; //2D matrix of nodes;
void addnode(int,int); //adds a node to matrix;
void removenode(int,int); //removes a node;
void printnode(ostream &out); //prints matrix;
private:

};

//This is my contructor, it sets all matrix values to zero;
adylist::adylist(int x,int y){
max = x*y;
for(int j=1;j <= max;j++){
for(int i=1;i<=max;i++){
nodearray[j] =0;
}
}

}

//This fuction takes in two nodes, if they are ady, it places one's in the
//corresponding positons of the matrix;
void adylist::addnode(int x,int y){
nodearray[x][y] =1;
nodearray[y][x] =1;

}

//This prints out the ady matrix to an output stream;
void adylist::printnode(ostream& out){
for(int j=1;j <=max;j++){
out<<j<<"\t ";

for(int i=1;i<= max;i++){
out <<nodearray[j]<<" ";
}

out<<"\n";
}

}

int main(){

int height,width,start,target,x,y, i = 1;
char in_file_name[36],out_file_name[36];

ifstream fin;
ofstream fout;

stack nodes,lines;


cout<<"Enter the input file name:\n";
cin>> in_file_name;
cout<<"Enter the output file name:\n";
cin>> out_file_name;


fin.open(in_file_name);

if(fin.fail()){
cout<<"connot open the input file.\n";
exit(1); //If the file fails to open, this exits the program;
}

fout.open(out_file_name);

if(fout.fail()){
cout<<"connot open the output file.\n";
exit(1); //If the file fails to open, this exits the program;
}

fin >>width;
fin >>height;
fin >>start;
fin >>target;


adylist maze(width,height); //creates the object maze and passes width
//and height to the constructor;


//This loope provides the data to the maze object;
while((fin.eof())!=true){
fin>>x>>y;
maze.addnode(x,y);
}
fin.close();


nodes.push(start);

/*This loope searches the adj matrix,when it finds a node that is linked, it
marks that node, and moves on to the linked node; It continues in this fashion,
untill it reaches the end of the ady list, or reaches a node that has no link, at which point
it goes back to the pervious node, and checks if there was another node linked to it, if not,
it goes back to the node before that again, and so on, untill it find another node which is linked
and therefore another route through the maze;
*/
for(int j =1; j <= maze.max ;j++){

//checks if node has a link to another row;
if(maze.nodearray[j] == 1){
lines.push(i); //adds node to a stack so we can remember were we've been;
nodes.push (j); //adds node to the route list;
maze.nodearray[j] =0; //marks the node by removing its link;
maze.nodearray[j] =0; //So no backtracking, but also no looping;
i = j; //sets the current node to the linked node;
j = 0; //sets the seacrh index back to 0;
}
/*Removes node from route if it has no linkes and isn't target
sets current node to pervious node;*/
else if((j == maze.max)&&(i!= maze.max)){
i = lines.pop();
nodes.pop();
j=0;
}

}


nodes.printstack(fout); //Prints out the route;
fout.close();
system("pause");
return 0;
} [/PHP]

Mutant_Fruit

untill it reaches the end of the ady list

Until is spelt "until" (you made that mistake a few times). And you typed ADJ once instead of ADY (i assume thats a mistake).

Other than that, it seems perfectly well commented. Basically, assume i now nothing of how programming works, and you want to comment each section telling me basically what it does. If you've done that, you've commented it correctly. You don't have to go into details, unless you've done something really crazy/inventive and want other programmers to easily understand your strange methods.

Syth

The purpose of commenting is to write down what you, the programmer, think the programme is doing. Programmes will do exactly what you told them to do, so if something doesn't work someone else can look at the code and find out what the programmer thought it was doing, and can figure out what the programme is actually doing, and find the difference (ideally there should be none).

Boston

Mutant_Fruit wrote:

Until is spelt "until" (you made that mistake a few times). And you typed ADJ once instead of ADY (i assume thats a mistake).

Other than that, it seems perfectly well commented. Basically, assume i now nothing of how programming works, and you want to comment each section telling me basically what it does. If you've done that, you've commented it correctly. You don't have to go into details, unless you've done something really crazy/inventive and want other programmers to easily understand your strange methods.

I don't think its fair to assume you know nothign about programming. If your looking at the scource code you should at least know the mechanics of c++; Apart from afew spelling mistakes (I was writing this at 11 last night) and indenting (looks fine in dev) it seems grand to moi.

Random

Comments are there to guide another able bodied programmer to know what your code does at a glance. There's nothing wrong with using language specific terms or anything similar in it.

Comments aren't there to teach someone how to programme in whatever language, they are there to highlight what might not be obvious at first glance.

str8_away

Hope you don't mind I be straight with my thoughts.

Only spend few minutes reading your code and commets.
I have no problem to understand what you are trying to do with your code from your comments.
But after tring to understand your code I found that your momments are missing some important information.

1) description is a bit short.
"Description: Path finder program. "
You could tell what method you are using to find the path, which is called "depth first" btw.
some information about the map would be great too. ie 2D, grid or box.
(if 2d map then each node should have only 4 max link but I am not sure about your map structure)
rule of maze, where to start, where to finish.
What information is stored in the input file and output file.


2)"int top; //Indicates the top postion in the stack;"
You could give a bit more information on what "top position" is and what 0 means and possible max value.

3)" int contents[500]; //Holds the values of the stack; "
I would move 500 out and set a const. So you can change this value easly without searching through the whole code for other "500"
Also why 500?

4)" int max; //Mazimum node value; "
Would be nice to know where this max valu came from rather to search and guess it is from x*y in adylist::adylist(int, int).

5) following comment and code does not make sence to me, as far as I understand your "world" or map this function only taking in one node.
//This fuction takes in two nodes, if they are ady, it places one's in the
//corresponding positons of the matrix;
void adylist::addnode(int x,int y){
nodearray[x][y] =1;
nodearray[y][x] =1;

}

6)Why are you using max as top limit of for loop?
In your map does every node is able to link to anynode in the map?
Also you have " int nodearray[500][500]; //2D matrix of nodes; "
which imply that max is 500 at max. but you does not have this warning in your comments.

//This is my contructor, it sets all matrix values to zero;
adylist::adylist(int x,int y){
max = x*y;
for(int j=1;j <= max;j++){
for(int i=1;i<=max;i++){
nodearray[j] =0;
}
}

}

7)"stack nodes,lines; "
What are lines?

I think I might be able to guyess what it is if I spend more time looking at your code but its late and I am tired.

I hope This is some help to you.

Valehru

Dont forget that there is such a thing as over-commenting...For christ sake it would probably take you about half the time you actually spent writing the program to explain it to somone who probably knows dick about programming. I seriously dont like over commenting my code...just stick to the bare necesscities.

Hobbes

Your comments are way too short for the functions/class.

instead of..

//add fuction;

Have

/* 
  Function: stack::full
  Returns: bool
  Errors thrown: None
  Description: Checks to see if top has reached 500 and returns true if it has. 
  Notes: Why am I using a fixed number instead of a constant? Also what happens if
  the code changes and it hits 501? 
*/

I know you can go on about how people should know C++. I don't. However if you get a good formatting for your functions and what they do you can create an API manual from the comments (in the same way javadoc works). Does C++ have something similar to javadoc?

Also when coding (even though it is small) assume that numerous people are going to be working on it with varying skills and may not read all of the code to see what everything is doing.

Also avoid comments like this...

exit(1);        //If the file fails to open, this exits the program;

Its obvious (at least to a C++ coder) that the program is exiting. You have verbal comment explaining it the line above as well as the IF statement is obvious it is checking a file.

If the check statement isn't obvious in C++ maybe comment that to say "Open file failed", but otherwise avoid putting comments that state the obvious, especially if the function you are calling has good commenting.