Cleaning up my code

I've written a pretty simple calculator program in C++ (haven't we all?), and I've worked out all the errors; it seems to run fine, except that if the answer to the problem is a large answer, the program keeps looping (which I set it up to do), but won't stop on cin's. I'm pretty new to C++, and I tried searching several times for clues as to the problem, but couldn't find anything. I know from seeing a million others get their asses ripped for posting entire programs, that you're not supposed to, but I don't know how else to explain my problem, and ask for any suggestions. So here's a good bit of my program. I don't expect you to rewrite my program, this is just practice for me anyway, but any suggestions on making my proggie shorter and more efficient would be appreciated. Thanks.


--LiKWiD
Becoming un-noobified one day at a time.

Code:

---

int op, rem;
int res = 1;
double x1, x2, x3, x4;
double tmpsum  = 0.1337;
double tmpsum2 = 0.1337;
double tmpsum3 = 0.1337;
char a1[2], a2[2], a3[2], problem[18];

char add[2] = "+";
char subtract[2] = "-";
char multiply[2] = "*";
char divide[2] = "/";

/* Organizes user input ('problem[18]') for processing */
 
void ioProb()
{
  ofstream oprob("problem.dat", ios::trunc); // Outputs user input to file
  oprob<<problem;
  oprob.close();
 
  ifstream iprob("problem.dat", ios::nocreate); // Inputs and organizes user input from file
  iprob>>x1>>a1>>x2>>a2>>x3>>a3>>x4>>rem>>rem;
  iprob.close();
}



/* Identifies and performs the math operation(s) */

void exeArith()
{
  cout<<"Please enter the problem you wish solved: "<<endl<<endl
      <<"              ";
  cin.getline(problem, 11, '\n');
  ioProb();
 
 
  op = strcmp(a1, add);
      if (op == 0)
      {
        tmpsum = x1 + x2;
      }
  op = strcmp(a1, subtract);
      if (op == 0)
      {
        tmpsum = x1 - x2;
      }
  op = strcmp(a1, multiply);
      if (op == 0)
      {
        tmpsum = x1 * x2;
      }

/* ... I cut out a lot of the IF statement here to shorten it some.. you get the point. */

  op = strcmp(a3, divide);
      if (op == 0)
      {
        tmpsum3 = tmpsum2 / x4;
      }
      if (tmpsum != 0.1337 && tmpsum2 != 0.1337 && tmpsum3 == 0.1337)
      {
        cout<<tmpsum2<<endl;
      }
      if (tmpsum3 != 0.1337)
      {
        cout<<tmpsum3<<endl;
      }
}



/* Calculates new problem or quits program. */

void newProb()
{
  cout<<endl<<endl
      <<"Solve another problem or quit?"<<endl
      <<" 1- Solve another"<<endl
      <<" 2- Quit"<<endl;
  cin>>res;
  cin.ignore();
}



/* MAIN() */

int main()
{
  cout<<"================================================"<<endl
      <<"--            Welcome to LiKWiD's            --"<<endl
      <<"--  ARITHMETIC  v1.0  -  (SYNTAX SPECIFIC)  --"<<endl
      <<"------------------------------------------------"<<endl<<endl
      <<"              Syntax:  x + y"<<endl
      <<"                      x - y"<<endl
      <<"                      x * y"<<endl
      <<"                      x / y"<<endl
      <<"                    x + y * z"<<endl<<endl<<endl;
 
  while (res == 1)
  {
    exeArith();
    newProb();
  }
 
  return 0;
}

---

LikWid, please make sure your subject lines are informative of your problem. You'll waste people's time if you're not specific.

Sorry, I don't mean to waste anybody's time. I really don't have a problem, I'm simply asking for suggestions if people have the time...


--LiKWiD
Becoming un-noobified one day at a time.

Before you post too many more threads, have a look at these:

http://www.catb.org/~esr/faqs/smart-questions.html

http://www.chiark.greenend.org.uk/~sgtatham/bugs.html

Cheers to Salem for those links.

Here are a few small suggestions.

Replace this:

Code:

---

char a1[2], a2[2], a3[2], problem[18];

char add[2] = "+";
char subtract[2] = "-";
char multiply[2] = "*";
char divide[2] = "/";

---

With this:

Code:

---

char a1, a2, a3, problem[18];

const char add = '+';
const char subtract = '-';
const char multiply = '*';
const char divide = '/';

---

Then, you can replace your mess of if statements with a switch:

Code:

---

switch(a1)
{
  case add:
    tmpsum = x1 + x2;
    break;
  case subtract:
    tmpsum = x1 - x2;
    break;
// continue with the rest of the ops
}

---

That should clean up the code a little bit.

There are, however, more efficient and flexible ways to go about this type of program. If you're in a learning and improving sort of mood, you can lookup tokenizing, parsing, and stacks. Using those concepts, you can process incredibly large expressions with relatively little code.

thefroggy, I think the strings need to be refined further. Your modification to LikWid's code will result in garbled output, because there is no null terminator. Unless of course, local stack chars are a special case. I am used to creating pointers to char arrays, so I'm not really sure. But if I'm right, I would do the following:

Code:

---

const char add[2]={'+',0}

---

Or something like that. Personally, this is my approach:

Code:

---

char *add=new char [2];
strcpy(add,"+");

delete [] add;

---

Hey, thanks for the suggestions.

If I use a switch, I still have to set up an IF similar to what I have to tell it which case to use, correct?

Ahh, pointers. I hadn't thought to use those. Would make it a bit easier (if I can remember how to use 'em :p).

Thanks, guys. (Do you have any idea why the program would crash/flood [whatever it should be called] like it does when I enter a problem with a large answer? [5 digits or more] Or is it the feeble coding?)

If you're just looking to make it take up fewer lines (this doesn't improve speed or anything, except for the elimination of the op variable) you could replace:

Code:

---

op = strcmp(a1, add);
    if (op == 0)
    {
        tmpsum = x1 + x2;
    }

---

with

Code:

---

if (strcmp(a1, add)==0)
    tmpsum = x1 + x2;

---

That turns 5 lines into 2, and (IMO) makes it a little easier to read.

Quote:

---

Originally posted by bennyandthejets
thefroggy, I think the strings need to be refined further. Your modification to LikWid's code will result in garbled output, because there is no null terminator. Unless of course, local stack chars are a special case. I am used to creating pointers to char arrays, so I'm not really sure. But if I'm right, I would do the following:

Code:

---

const char add[2]={'+',0}

---

---

Like you said, this creates a character array and doesn't alleviate the if and strcmp statements. My approach just used a single character. There will be no garbled output for a few reasons:

1. Single characters are not character arrays, and therefore, do not need null terminators.

2. Those variables are not used for output-- only for comparison to determine which operation to use.

A single char is simply treated as an integral type and by making it const, we can use it as a case label in a switch statement.

Quote:

---

If I use a switch, I still have to set up an IF similar to what I have to tell it which case to use, correct?

---

No, the switch will replace the if statements. Here is an if with its equivalent switch:

if:

Code:

---

if(i == 1)
{
  std::cout << "one" << std::endl;
}
else if(i == 3)
{
  std::cout << "three" << std::endl;
}
else
{
  std::cout << "not one or three" << std::endl;
}

---

switch:

Code:

---

switch(i)
{
  case 1:
    std::cout << "one" << std::endl;
    break;
  case 3:
    std::cout << "three" << std::endl;
    break;
  default:
    std::cout << "not one or three" << std::endl;
    break;
}

---

Each of those will work the same way.

Ok, I changed up a lot of the code. Thanks to froggy I have...

Code:

---

const char add = '+';
const char subtract = '-';
const char multiply = '*';
const char divide = '/';

---

and the switch statement you suggested.

Code:

---

switch (a1)                                // Performs math operations on x1 & x2
    {
      case add:
          tmpsum = x1 + x2;
          break;
      case subtract:
          tmpsum = x1 - x2;
          break;
      case multiply:
          tmpsum = x1 * x2;
          break;
      case divide:
          tmpsum = x1 / x2;
          break;
      default:
          cout<<"Syntax incorrect. Please retry...(remember spaces between characters)"<<endl;
          break;
    }

---

It works, and cuts down a lot of lines. Thanks again!

However, my program still loops indefinitely when I figure a large problem. It comes up with the correct answer, but just continues looping, showing the answer, etc., w/o ever asking to quit.

I've attached the .cpp if you want to try it; maybe that'll help you understand what it's doing.

If it is only happening with very large numbers, then I suspect buffer overflow is your problem. I can't think of a simple way to work around that without changing the program design, but it should still work, as is, for smaller numbers.

Also, I noticed this:

Code:

---

iprob>>x1>>a1>>x2>>a2>>x3>>a3>>x4>>rem>>rem;

---

Why do you read in rem twice in the same line? Also, I'm not sure if the behavior for modifying it twice like that is defined.

Well, I removed rem when I changed it up a bit. I had it in there before to basically discard a couple extra variables the person may have entered (since my program can only calculate 4 variables).

e.g., user input: 1 + 2 + 3 + 4 + 5

the last + and then 5 would become rem, which I would simply ignore, and figure the first four variables. Anyway, that part is gone now. To see what I have now, you can d/l the source.

Btw, how would I go about 'preventing a buffer overflow', if that is, indeed, the problem?

Alright, sorry 'bout that. I was too lazy to download it earlier.

I see two good solutions... neither one particularly easy. The first is to use an arbitrary precision data type (a class, for which some libraries do exist). The second would be to read all of your data in as a single string, and then to parse that string, and do some checking as to whether or not the numbers will be too large for your data types (double), and then acting accordingly.

This is a bit of an advanced technique, but you can use a pointer to a function to clean up your operand code.

For example:

Code:

---

#include <iostream.h>
#include <stdlib.h>

int addition (int a, int b)
{ return (a+b); }

int subtraction (int a, int b)
{ return (a-b); }

int main()
{

int (*operation)(int,int);

operation = addition;

cout << operation(3, 4) << endl;
cout << operation(9, 5) << endl;

operation = subtraction;

cout << operation(3, 4) << endl;
cout << operation(9, 5) << endl;

      system("PAUSE");
      return 0;
}

---

the int (*operation)(int,int) line creates a pointer to a function that returns an integer and takes two integer values.

the line operation = subtraction assigns the pointer to that function.

Neat-o!

--Ashiq

Warning: long code ahead...


This is a simple expression parser that I threw together in about 10 minutes. It should evaluate expressions of any length and it will handle correct order of operations. The code includes a lexical scanner, a parser, and a calculate function that makes use of a value stack.

Code:

---

#include <locale>
#include <iostream>
#include <sstream>
#include <string>
#include <vector>


using namespace std;

enum TokenType { T_ADD, T_SUBTRACT, T_DIVIDE, T_MULTIPLY, T_NUMBER, T_EOS };


struct token
{
  TokenType type;
  double value;
};


class Scanner
{
public:
  void
  init(istream* is_)
  {
    is = is_;
    next();
  }

  token
  scan()
  {
    token t;

    switch(current)
    {
      case T_EOS:
      case ';':
        t.type = T_EOS;
        return t;
      case '+':
        t.type = T_ADD;
        next();
        return t;
      case '-':
        t.type = T_SUBTRACT;
        next();
        return t;
      case '*':
        t.type = T_MULTIPLY;
        next();
        return t;
      case '/':
        t.type = T_DIVIDE;
        next();
        return t;
      default:
        if(isdigit(current))
        {
          t.type = T_NUMBER;
          t.value = getNumber();
          return t;
        }
        throw string("invalid token");
    }
  }

private:
  istream *is;
  char current;
  ostringstream buffer;

  void
  next()
  {
    if(is->good())
      (*is) >> current;
    else
      current = T_EOS;
  }

  double
  getNumber()
  {
    bool foundDecimal = false;

    buffer.str(string());

    while((isdigit(current) || current == '.'))
    {
      if(current == '.')
      {
        if(foundDecimal)
          throw string("malformed number");
        foundDecimal = true;
      }

      buffer << current;
      next();
    }

    return atof(buffer.str().c_str());
  }
};


class Parser
{
public:
  vector<token>
  parse(istream* is)
  {
    scanner.init(is);
    tokenSetup = false;
    tokens.clear();
    next();
    expression();
    return tokens;
  }

private:
  Scanner scanner;
  vector<token> tokens;
  token current, lookAhead;
  bool tokenSetup;

  void
  next()
  {
    current = lookAhead;
    lookAhead = scanner.scan();

    if(!tokenSetup)
    {
      tokenSetup = true;
      next();
    }
  }

  void
  expression() { additive_expression(); }

  void
  additive_expression()
  {
    multiplicative_expression();

    while(lookAhead.type == T_ADD || lookAhead.type == T_SUBTRACT)
    {
      token op = lookAhead;
      next(); next();
      multiplicative_expression();
      tokens.push_back(op);
    }             
  }

  void
  multiplicative_expression()
  {
    primary_expression();

    while(lookAhead.type == T_MULTIPLY || lookAhead.type == T_DIVIDE)
    {
      token op = lookAhead;
      next(); next();
      primary_expression();
      tokens.push_back(op);
    }     
  }

  void
  primary_expression()
  {
    if(current.type != T_NUMBER)
      throw string("expected number");

    tokens.push_back(current);
  }


};


double Calculate(string expression)
{
  vector<token>  tokens;
  vector<double>  valueStack;
  Parser          parser;

  expression += ";";

  istringstream iss(expression);

  try
  {
    tokens = parser.parse(&iss);
  }
  catch(string err)
  {
    cerr << "Error: " << err << endl;
    return 0;
  }

  vector<token>::iterator itr = tokens.begin();

  for( ; itr != tokens.end(); itr++)
  {
    token t = *itr;

    if(t.type == T_NUMBER)
      valueStack.push_back(t.value);
    else
    {
      double lhs, rhs, result;

      rhs = valueStack.back();
      valueStack.pop_back();
      lhs = valueStack.back();
      valueStack.pop_back();

      switch(t.type)
      {
      case T_ADD: result = lhs + rhs; break;
      case T_SUBTRACT: result = lhs - rhs; break;
      case T_MULTIPLY: result = lhs * rhs; break;
      case T_DIVIDE: result = lhs / rhs; break;
      default: break;
      }

      valueStack.push_back(result);
    }

  }
   
  return valueStack.back();
}


int main()
{
  string expression, answer;

  do
  {
    cout << "Enter an expression: " << flush;
    getline(cin, expression, '\n');
    cout << "Value is: " << Calculate(expression) << endl;

    cout << "Enter another? (yes/no) " << flush;
    getline(cin, answer, '\n');

  } while(answer[0] == 'y');

  return 0;
}

---

I'm just providing this to show you a traditional method for creating a simple calculator.

Quote:

---

A single char is simply treated as an integral type and by making it const, we can use it as a case label in a switch statement.

---

I don't understand why it must be const.

Quote:

---

Originally posted by bennyandthejets
I don't understand why it must be const.

---

It's just the design of the switch statement. Each case label must have a constant value. By restricting the cases to constant values, it allows the compiler to make more aggressive optimizations on the jump table.

Try this in your compiler:

Code:

---

#include <iostream>

using namespace std;

int main()
{
  int one = 1;
  int two = 2;

  int val = 1;

  switch(val)
  {
  case one: cout << "one" << endl; break;
  case two: cout << "two" << endl; break;
  }

  return 0;
}

---

I get the following errors:

$ g++ test.cpp
test.cpp: In function `int main()':
test.cpp:14: case label does not reduce to an integer constant
test.cpp:15: case label does not reduce to an integer constant

However, change it to this:

Code:

---

#include <iostream>

using namespace std;

int main()
{
  const int one = 1;
  const int two = 2;

  int val = 1;

  switch(val)
  {
  case one: cout << "one" << endl; break;
  case two: cout << "two" << endl; break;
  }

  return 0;
}

---

And it compiles without errors.