2
\$\begingroup\$

My postfix program essentially allows the user two input options to calculate a postfix equation:

  • Confirmations after each input (after an input, the user is ask for another number, operator, or final result)
  • No confirmation after each input (user is only asked for a number of inputs (values and operators), and the final result is shown after the loop)

My original attempt at this program used the first option because I had trouble getting my program to tell the difference between a value and an operator (both are treated as ASCII values, which is normal). However, I used a better method (the second option) after learning about atoi(). Alongside these two functions, another function does all of the calculations for both functions.

How could this program be improved for clarity and effectiveness? Should I even bother offering both options, even just for comparison's sake? I'm also not sure how much input validation I should provide, if it's really necessary.

NOTE: The IntStack class was written by an instructor of mine, and I'm just using it for simplicity and learning. As it is part of my own written code, I'm including it here so that my code's meaning is not lost. As it's not my own code, please do not review it. Only review my own code.

Postfix.cpp

#include <iostream>
#include <cstdlib>
#include "IntStack.h"
using std::cout;
using std::cin;

int noConfirmationExecution();
int confirmationExecution();
void calcOperation(IntStack&, char);

int main()
{
    int executionType;
    int calcTotal;

    cout << ">>> Execution Type:\n\n";
    cout << "(1) Values entered all at once (with no confirmations)\n";
    cout << "(2) Values entered one at a time (with confirmations)\n\n";
    cin >> executionType;

    if (executionType == 1)
        calcTotal = noConfirmationExecution();
    else if (executionType == 2)
        calcTotal = confirmationExecution();

    cout << "\n\n*** Total: " << calcTotal << "\n\n\n";

    system("PAUSE");
    return 0;
}

int noConfirmationExecution()
{
    IntStack stack(20);

    char item = ' ';

    cout << "\n>> Items ('s' to stop):\n\n";

    cin >> item;

        while (item != 's')
        {
            int i = atoi(&item);

            if (item != '+' && item != '-' && item != '*' && item != '/')
                stack.push(i);
            else if (item == '+' || item == '-' || item == '*' || item == '/')
                calcOperation(stack, item);

            cin >> item;
        }

    int total;
    stack.pop(total);
    return total;
}

int confirmationExecution()
{
    IntStack stack(20);

    int number;
    char _continue = 'Y';
    char anotherNumber;
    char anotherOperator;

    cout << "\n> Number: ";
    cin >> number;
    stack.push(number);

        do
        {
            do
            {
                anotherNumber = 'n';

                cout << "\n> Number: ";
                cin >> number;
                stack.push(number);

                cout << "\n- Another Number (y/n)? ";
                cin >> anotherNumber;

            } while (anotherNumber != 'n');

            do
            {
                anotherOperator = 'n';

                char _operator;
                cout << "\n>> Operator: ";
                cin >> _operator;
                calcOperation(stack, _operator);

                cout << "\n- Another Operator (y/n)? ";
                cin >> anotherOperator;

            } while (anotherOperator != 'n');

            cout << "\n- Continue (y/n)? ";
            cin >> _continue;

        } while (_continue != 'n');

    int total;
    stack.pop(total);
    return total;
}

void calcOperation(IntStack &stack, char _operator)
{
    int num1, num2;

    stack.pop(num2);
    stack.pop(num1);

    if (_operator == '+')
        stack.push(num1 + num2);
    else if (_operator == '-')
        stack.push(num1 - num2);
    else if (_operator == '*')
        stack.push(num1 * num2);
    else if (_operator == '/')
        stack.push(num1 / num2);
}

IntStack.h

#ifndef INTSTACK_H
#define INTSTACK_H

class IntStack
{
private:
   int *stackArray;
   int stackSize;
   int top;

public:
   IntStack(int);
   IntStack(const IntStack &);
   ~IntStack();
   void push(int);
   void pop(int &);
   bool isFull() const;
   bool isEmpty() const;
};

#endif

IntStack.cpp

#include <iostream>
#include "IntStack.h"
using namespace std;

IntStack::IntStack(int size)
{
    stackSize = size;
    stackArray = new int[size];
    top = -1;
}

IntStack::IntStack(const IntStack &obj)
{
   // Create the stack array.
   if (obj.stackSize > 0)
      stackArray = new int[obj.stackSize];
   else
      stackArray = NULL;

   // Copy the stackSize attribute.
   stackSize = obj.stackSize;

   // Copy the stack contents.
   for (int count = 0; count < stackSize; count++)
      stackArray[count] = obj.stackArray[count];

   // Set the top of the stack.
   top = obj.top;
}

IntStack::~IntStack()
{delete [] stackArray;}

void IntStack::push(int num)
{
   if (isFull())
   {
      cout << "The stack is full.\n";
   }
   else
   {
      top++;
      stackArray[top] = num;
   }
}

void IntStack::pop(int &num)
{
   if (isEmpty())
   {
      cout << "The stack is empty.\n";
   }
   else
   {
      num = stackArray[top];
      top--;
   }
}

bool IntStack::isFull() const
{
   bool status;

   if (top == stackSize - 1)
      status = true;
   else
      status = false;

   return status;
}

bool IntStack::isEmpty() const
{
   bool status;

   if (top == -1)
      status = true;
   else 
      status = false;

   return status;
}
\$\endgroup\$
2
\$\begingroup\$

Sure if you have too:

using std::cout;
using std::cin;

But are you really saving that much.

Even forward declaration of functions, its nice to have the name of the parameters. It helps in understanding the context in which the function will be called.

void calcOperation(IntStack&, char);

I leave the parameter names off when they are not used (so usually this only happens in overridden virtual functions).

What happens if the user enters not one or two?
Personally I would make this a command line flag. Under normal conditions just run without validation. With the command line flag '-h' it means a human is working interactively and thus will confirm values.

Very platform specific:

    system("PAUSE");

There is no real need for this. In a terminal the application quitting is not a problem. In an IDE you just have to set a configuration option to stop the window closing. So you really should not need to bother with this.

Personally I only return a value from main() if there is an option of failing.

    return 0;

If the application always succeeds then no return is an indication that it will always work and thus no need to check for errors (note: in C++ main() is special and the compiler inserts return 0 if it does not exist).

Since you use the same stack in both situations. Why not declare it in main() and pass it in. Then if you need to modify it you only have one place to change (not two as in the current implementation).

    IntStack stack(20);

Since item is a char. Any numbers can only be single digit. So using atoi() seems a waste. But it is also wrong. Because atoi() expects a '\0' terminated C-String (you have not provided this you need a second character set to '\0'

    cin >> item;

        while (item != 's')
        {
            int i = atoi(&item);

To make this work you need:

    cin >> item;
    while (item != 's)
    {
        int i = item - '0';

But to make it work with multi-characer numbers I think I would use std::string

    std::string word;
    std::cin >> word;
    while (item != "s")
    {
        try
        {
            int i = boost::lexical_cast<int>(word);
            stack.push(i);
        }
        catch(...)
        {
           // handle + - * / here as they are not numbers;
        }

If you can't use boost then the same affect can be achieved with std::stringstream and a little more work.

Your while loop looks like a for(;;) loop. So why not use it:

        cin >> item;
        while(item != 's')
        {
            // STUFF
            cin >> item;
        }

        // I would write like this:
        for(cin >> item; item != 's'; cin >> item)
        {
        }
\$\endgroup\$
  • \$\begingroup\$ I primary use Visual C++, so I do need system("PAUSE"). I am aware that it doesn't work on other platforms (I've coded C++ on Linux before). I was always told that I don't need to put variable names in prototypes, but I can do that anyway. I can also leave out return 0 from now on, in those cases you've mentioned. I have also put the stack initialization in main(). I was unaware of the alternate syntax for cin (at the bottom), and I'll keep that in mind. I love learning about alternate syntax forms. \$\endgroup\$ – Jamal Mar 25 '13 at 1:15
  • \$\begingroup\$ I may need some additional help with the input (where you mentioned boost and stringstream). I apparently cannot use boost, but I can use stringstream. I just need to figure out how to use it with numbers and operators (going back to atoi, which I mentioned before). \$\endgroup\$ – Jamal Mar 25 '13 at 1:18
  • \$\begingroup\$ @JamalA: See third answer below. \$\endgroup\$ – Martin York Apr 1 '13 at 3:04
2
\$\begingroup\$

Convert string to any type:

template<typename ResultType>
ResultType lexical_cast(std::string const& input, bool useAll = true)
{
    std::stringstream inputstream(input);
    ResultType result;

    // Try and convert.
    // A failed conversion results in an exception.
    if (!inputstream >> result)
    {    throw std::runtime_error("Failed to convert");
    }

    char x;
    if (useAll && inputstream >> x)
    {
         // If the above operations worked.
         // This means that there was more data in the input string
         // that was not used in the conversion to ResultType
         // 
         // Say the input string was 5.2
         // If you convert this to int then the `result` is 5
         // but you have left ".2" on the input stream this may be a problem.
         // So this is a test for non white space characters left on the input.

         throw std::runtime_error("Not all data used in conversion");
    }

    return result;
}

Usage:

int   x = lexical_cast<int>("5");
float y = lexical_cast<float>("5.678");

int   x = lexical_cast<int>("5.2", false); // false shows you don't
                                           // care about left over data.
\$\endgroup\$
  • \$\begingroup\$ If you want it to work with just ints. Then remove the line template<typename ResultType> and replace all occurences of ResultType with int \$\endgroup\$ – Martin York Apr 1 '13 at 3:55
  • \$\begingroup\$ Okay, I'll try that. Please take a look at the updated code in case there are other problems. I apologize for this request becoming off-topic for this site. It doesn't seem necessary to duplicate it on Stack Overflow, though. \$\endgroup\$ – Jamal Apr 1 '13 at 3:55
  • \$\begingroup\$ Okay, I found the problem. The pop() function in MyStack had a bug, but I fixed it. I tested the program after that, and everything worked. My original stringstream was correct as well, allowing me to use multi-digit numbers. I'll change that line in the code and finally put this case to a close. \$\endgroup\$ – Jamal Apr 1 '13 at 18:14
1
\$\begingroup\$

Here are what I think.

  • Don't use system("PAUSE"), you can easily implement this functionality. It might works fine on Windows system, but will not work on Mac, and Linux.
  • When the stack is full, your program returned an incorrect result. If you have learned about exception handling, this is a good opportunity to use it. Instead of print out an error message, raise an exception and let the caller (calcTotal) handle it.
  • In general, you should spell out an identifier (function name, variable name, …) instead of abbreviate it. For example convertedNumber is better than convItem.
  • More about convItem: please name your identifier to reflect its role. I suggest using the name operand instead of convItem. Similarly, operand1, operand2 are better than num1, num2, respectively.
  • Use of blank lines: you should use blank lines to stragically organize your code (to separate out units such as functions, logical block of codes, …) However, too many blank lines make the code hard to follow due to the constant need of scrolling.
  • Avoid using hard-coded constants such as "+", you should use constants with descriptive names such as OPERATOR_ADD, OPERATOR_SUBTRACT…
  • Right now, your code assumes the items are just numbers, four operators and ignore any invalid tokens such as sin, cos, … It is better to flag them as errors than to ignore them.
  • Keep in mind that using std::istringstream to convert a string to int is not robust: Try the following:

    std::string buffer = "Hello";
    int portNumber = -1;
    std::istringstream(buffer) >> portNumber; // failed, portNumber still = -1
    

Update: About system("PAUSE")

It seems the purpose of the pause command is to prevent Visual Studio from closing down the console. If that is the case, take a look at this stackoverflow discussion.

\$\endgroup\$
  • \$\begingroup\$ I only use system("PAUSE") when I'm on Windows. I do remember to take out when I put it on another OS. As for everything else, I'll make the necessary changes and post them again later. Thanks! \$\endgroup\$ – Jamal Apr 1 '13 at 19:53
  • \$\begingroup\$ Post updated. I have kept system("PAUSE") because it's just a hassle to find a better solution for it on Windows. The program has passed all of my testing, except -1 cannot be used for calculations (it's currently used for the error-handling). Is there an alternative, or do I just have to deal with one number (any number) being an invalid input? \$\endgroup\$ – Jamal Apr 1 '13 at 23:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.