Expert: vijayan - 3/3/2009

Question
Could you please critique my code?
It keeps getting wrong output, and I don't know why.
It is a program that can be executed by using command line arguments in the form (./a.out -c -n).  
C could be even or odd, and N is the integer.

Here is my code so far:
#include <iostream>
#include <string>
using namespace std;

int main(int argc, char *argv[])
{
 string number = "";
 
 if (argc !=3)
   {
     cout << "Usage: prog1 -c -n "<< endl;
   }
 
 else
   {
     number = argv[2][1];
     int n = atoi(number.c_str());
     int sum = 0;

     if(argv[1][1] == 'E' || argv[1][1] == 'e')
       {
         for (int i=0; i<=n; i++)
           {
             if (i%n==0)
               sum = sum + i;
           }
         cout << "Sum of even integers: " << sum << endl;
       }
     
     else if(argv[1][1] == 'O' || argv[1][1] == 'o')
       {
         for (int i=0; i<=n; i++)
           {
             if (i%n!=0)
               sum = sum + i;
           }
         cout << "Sum of odd integers: " << sum << endl;
       }
   }
 return 0;
}   


Ouput:
./a.out -e -4
Sum of integers: 6

But I get this output instead:
./a.out -e -4
Sum of integers: 4

Could you please help me?
Thanks.

Answer
one immediate problem i can see in your code is:

for (int i=0; i<=n; i++)
{
        if( i%n == 0 ) // **** check for even i


and likewise,

for (int i=0; i<=n; i++)
{
        if( i%n != 0 ) // **** check for odd i


the check should be against i % 2 ( not i % n ).


a second problem, which you may want to fix is the use of atoi().
atoi() doesn't allow much in the way of error detection and recovery. it silently results in undefined behavior on overflow and there are input conditions that cause it to fail unpredictably.

the standard C++ alternative is to construct a stringstream and read it's data into an int using >>. you could also use a lexical_cast(), see: http://www.boost.org/libs/conversion/lexical_cast.htm

the atoi() function has been deprecated by strtol(). even in pure C code, prefer using strtol(); it is much better behaved.