Thread: Cleaning up my code

  1. #1
    Registered User
    Join Date
    Jul 2003
    Posts
    26

    Post 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;
    }

  2. #2
    mustang benny bennyandthejets's Avatar
    Join Date
    Jul 2002
    Posts
    1,401
    LikWid, please make sure your subject lines are informative of your problem. You'll waste people's time if you're not specific.
    [email protected]
    Microsoft Visual Studio .NET 2003 Enterprise Architect
    Windows XP Pro

    Code Tags
    Programming FAQ
    Tutorials

  3. #3
    Registered User
    Join Date
    Jul 2003
    Posts
    26
    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.

  4. #4
    mustang benny bennyandthejets's Avatar
    Join Date
    Jul 2002
    Posts
    1,401
    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.
    [email protected]
    Microsoft Visual Studio .NET 2003 Enterprise Architect
    Windows XP Pro

    Code Tags
    Programming FAQ
    Tutorials

  5. #5
    Registered User
    Join Date
    May 2003
    Posts
    161
    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.
    Last edited by thefroggy; 07-07-2003 at 02:35 AM.

  6. #6
    mustang benny bennyandthejets's Avatar
    Join Date
    Jul 2002
    Posts
    1,401
    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;
    Last edited by bennyandthejets; 07-07-2003 at 02:57 AM.
    [email protected]
    Microsoft Visual Studio .NET 2003 Enterprise Architect
    Windows XP Pro

    Code Tags
    Programming FAQ
    Tutorials

  7. #7
    Registered User
    Join Date
    Jul 2003
    Posts
    26
    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 ).

    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?)
    --LiKWiD
    Becoming un-noobified one day at a time.

  8. #8
    Pursuing knowledge confuted's Avatar
    Join Date
    Jun 2002
    Posts
    1,916
    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.
    Away.

  9. #9
    Registered User
    Join Date
    May 2003
    Posts
    161
    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.


    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.

  10. #10
    Registered User
    Join Date
    Jul 2003
    Posts
    26
    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.
    --LiKWiD
    Becoming un-noobified one day at a time.

  11. #11
    Toaster Zach L.'s Avatar
    Join Date
    Aug 2001
    Posts
    2,686
    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.
    The word rap as it applies to music is the result of a peculiar phonological rule which has stripped the word of its initial voiceless velar stop.

  12. #12
    Registered User
    Join Date
    Jul 2003
    Posts
    26
    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?
    Last edited by LiKWiD; 07-07-2003 at 02:13 PM.
    --LiKWiD
    Becoming un-noobified one day at a time.

  13. #13
    Toaster Zach L.'s Avatar
    Join Date
    Aug 2001
    Posts
    2,686
    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.
    The word rap as it applies to music is the result of a peculiar phonological rule which has stripped the word of its initial voiceless velar stop.

  14. #14
    Registered User
    Join Date
    May 2003
    Posts
    37
    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

  15. #15
    Registered User
    Join Date
    May 2003
    Posts
    161
    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.
    Last edited by thefroggy; 07-07-2003 at 05:55 PM.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Proposal: Code colouring
    By Perspective in forum A Brief History of Cprogramming.com
    Replies: 28
    Last Post: 05-14-2007, 07:23 AM
  2. Explain this C code in english
    By soadlink in forum C Programming
    Replies: 16
    Last Post: 08-31-2006, 12:48 AM
  3. Updated sound engine code
    By VirtualAce in forum Game Programming
    Replies: 8
    Last Post: 11-18-2004, 12:38 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM