Skip to main content
Code Review

Return to Revisions

2 of 6
added 692 characters in body
JDługosz
  • 11.7k
  • 19
  • 40

#Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc.


Did you know that there is a stack container adaptor in the standard library? And basing it on a linked list is slow — use a vector.

I'll note some things about the Stack code for general teaching, since you should actually not write any of that at all.

Stack::Stack()
{
 top = NULL;
}

Don’t assign values in the body of the constructor; use an initializer list.

This one could be a default initializer (with the top field definition) and then you don’t write the constructor at all.

But you should not have naked new (and delete). So top should actually be a unique_ptr which knows how to initialize itself so you don’t need to do anything.

And, don’t use the C NULL macro.


char expression[80]; //Expression to be read from the keyboard

Don't hard-code array sizes or use them like this. The function should be written as

std::string read_expression()

check_expression(expression, valid);

No, return results. And declare variables where used and initialized:

bool valid = check_expression(expression);

Using "out" parameter instead of returns is bad; for no reason is just horrible.

Your parameters taking arrays probably don't mean what you think. In any case, pass std:string (or string_view which can be especially handy for parsing).


return (top == NULL);

don’t use the C NULL macro. But, don’t write explicit tests against nullptr’. Use the contextual conversion to bool`, which works efficiently for raw pointers and any kind of smart pointer instance.


bool Stack::IsFull() const
{
 int* p;
 return (!(p = new int));
}

That makes no sence! You leak memory, and always return true since new throws an exception on failure. How can a linked list be full, anyway?


It’s good to use typedefs instead of the actual type, for clarity in reading the code as well as for ease in changing things.

typedef char StackType;

But, typedef is obsolete and limited. Today, write instead

using StackType = char;

But don’t (generally) typedef pointers to other things. Read and understand something* as a pointer to something, not needing another unique name like somethingpointer instead.


You don’t need a return at the end of a void function.

JDługosz
  • 11.7k
  • 19
  • 40
default

AltStyle によって変換されたページ (->オリジナル) /