Return to Answer

• 1

• 97.7k
• 5
• 126
• 341

##Design

You should not expose pointers in C++.

void setHead(charNode* h);
charNode* getHead();

Pointers have no ownership semantics associated with them. Ownership is the concept of who is responsible for deleting a pointer (releasing a resource). If you have a pointer you can't tell (without reading the code/documentation) who is responsible for freeing it (or how to free it or if it is even freeable).

In the above interface I can quite easily break the program by doing:

charNode x('R');
list.setHead(&x); // pass the address of 'x' rather than
 // dynamically allocated object.

If you are using dynamically allocated memory you should be using some form of smart pointer to indicate the ownership of the object. If the object is not dynamically allocated then you can use a reference rather than a pointer.

But in this case I would not even expose this interface as it introduces coupling (by exposing the internal implementation of the class).

Your internal structure charStack is exposed as a public class and you return a pointer to this type of object via getHead(). This means you will need to support both of these indefinitely. This may prevent you from improving the class in the future (as both of these will need to be maintained to keep old code working).

The internal structure of a class should be kept private. This will reduce coupling with external code and allow you to change the underlying implementation if you discover a better technique without having to modify any of the code that uses your class.

Naming conventions. There is no absolute standard for C++ (so take as you require). But a common convention is that user defined classes begin with an upper case letter. The names of objects (variables/functions) begin with a lower case letter. This makes it easy to quickly identify what is a type and what is an object (which is very useful in a language where type information is paramount).

Your comments are bad. Comments need to tell me something that is not obvious from the code. Also the code should be self documenting. Thus usually comments should tell me why (or describe in detail the algorithm that is being used). I can read code so the comments should not repeat the code (otherwise you have to maintain the code and the comments to make sure they say the same thing (and that is extra work)).

The worst thing in development is finding code that does one thing and the comments say something else. Which is correct (the comments or the code). Which do I fix?

So minimize your comments and only write stuff I can not tell from reading the code.

##Code Review

Why are you publically exposing methods that can break the state of the object.

 void decCounter(); //decrements counter of nodes
 void incCounter(); //increments counter of nodes

I can increment/decrement the count of nodes, without changing the actual number of nodes in the class?

In this case it's not bad; but:

char pop(); //removes top node and returns its character value

But in the general case where the return value can be any type. This is not an exception safe method. This is why in standard containers pop() simply removes the top item, and there is another method top() for getting a copy of the top element. You may want to copy this pattern to be consistent with standard types.

Don't use this

charNode::charNode() {this->next=NULL;}

The need to use this is only required if we have shadowed variables. Shadowed variables are dangerous as you can accidently forget to use this and then you will be changing the wrong variable (and how do you tell if you should be using a local or a member variable if they have the same name)!

Shadowed variables are bad practice and can be detected by the compiler (and thus removed). When you have removed shadowed variables (because you code compiles with no errors) there is no longer any need to use this.

The use of this only creates problems so don't use it.

Only put one statement per line.

void charQueue::add(char c) 
{
 if(rear==NULL) {rear= new charNode(c); front=rear; counter=1; return;}
 charNode* tmp=new charNode(c); tmp->setNext(rear); rear=tmp; this->incCounter();
}

This is hard to read (and its a very short function).

void charQueue::add(char c) 
{
 if(rear==NULL)
 {
 rear= new charNode(c);
 front=rear;
 counter=1; 
 return;
 }
 charNode* tmp=new charNode(c);
 tmp->setNext(rear);
 rear=tmp;
 this->incCounter();
}

Now that I can see the code. I see a lot of common elements (and I am not sure I believe it is correct). Looks like the tmp element is being set as the second last element in the list and rear points to this element.