Assigning values from argv[]

[London Fog]

Hi,

I'm currently working on a program that accepts user input on the command line then outputs the mean, median,
mode, and variance of the data. I'm not concerned with how to do the calculations for the output, however when I'm encountering a problem when I try to use atoi() to assign the strings entered to a single dimensional array to be passed to the function to put all the numbers entered into order. At this point the only function I've finished is sort() which sorts them into order.

Example of ideal i/o:

Input: ./mmm 15 18 13 17 18
Output: 15 18 13 17 18

Example of actual i/o:

Input: ./mmm 15 18 13 17 18
Output: 0 0 0 0 0 0

Thanks for any help you can give. And I apologize if it's not the best written program, I'm still a beginnning programmer and am bound to make a lot of mistakes.

- London Fog

int main( int argc, char *argv[])
{
	int args_main[MAX]; /* Array used to hold user input. (MAX is defined as 128 in common.h) */
	int *args_main_p = &args_main[0]; /* Pointer used for allocation of memory */
	int control_1  = 0, control_2 = 1; /* Variable used for program control (ie. For loops etc) */

	args_main_p = (int*) malloc(MAX*sizeof(int)); /* Allocating memory for user input */

	if( args_main_p != NULL ) /* If memory allocates successfully */
	{

		for( control_1 = 0 ; control_1 < MAX ; control_1++) // Initializing array
		{
			args_main[control_1] = 0;

		}
	
		if( argc < 2 ) /* If no arguments are entered */
		{
			printf("\n Enter --help for help. (Duh)\n\n");
			exit(1);
		}
	
		if( argv[1][0] == '-' && argv[1][1] == '-' && argv[1][2] == 'h' && argv[1][3] == 'e' && argv[1][4] =='l' )
		{
			printf("\n Enter at least between 2 and 128 integers seperated by spaces.");
			printf("\n The program will output the mean, median, mode");
			printf("\n and variance of those integers.");
			printf("\n Ex Input: 15 18 13 17 18 \n");
			printf("\n Ex Output: \n mean: 16.2 \n median: 17 \n mode: 18 \n variance: 0 \n");
			exit(1);
		}

		do
		{
			args_main[control_1] = atoi(argv[control_2]);
			control_1++; control_2++;
		} while( atoi(argv[control_2]) >= 1 && atoi(argv[control_2]) <= 9 ); 	

		control_1 = 0;
			
		do						// Checking to make sure user
		{					       // values were passed to args_main
			printf("%d  ", args_main[control_1]); // correctly
			control_1++;
		} while( control_1 < argc-1 );	
	
		printf("\n Calling function sort() ");
	
		sort( args_main, argc ); /* Call function sort */
		
		return 0;
	}
	else /* If memory allocation fails */
	{
		printf("\n Memory allocation err. Killing program.\n");
		exit(-1);
	}
}

[nadroj]

		for( control_1 = 0 ; control_1 < MAX ; control_1++) // Initializing array
		{
			args_main[control_1] = 0;

		}

So once this for loop is done, control_1 has value MAX (which you say is 128). Also, the args_main array has values of 0, from indices 0 to 128. Next:

		do
		{
			args_main[control_1] = atoi(argv[control_2]);
			control_1++; control_2++;
		} while( atoi(argv[control_2]) >= 1 && atoi(argv[control_2]) <= 9 );

Your assigning values of 'args_main' with indices 128 to who knows? You probably want to go from 0 to argc. Finally

		control_1 = 0;
			
		do						// Checking to make sure user
		{					       // values were passed to args_main
			printf("%d  ", args_main[control_1]); // correctly
			control_1++;
		} while( control_1 < argc-1 );

Your printing 'args_main' from indices 0 to argc-1, all of which (as described above) have values 0.

EDIT: Also, you need to free the memory you allocated with malloc, before the return. Otherwise theres a memory leak of 128bytes each time you run it.

[jeffcobb]

Greetings oh foggy one.

Depending on your platform you could probably benefit from getopt. Getopt automates the parsing and use of command line switches in your application. Here is an example of it in action:

using namespace std;
// h - help
// b - binary transfer test
// m <'m' | 's'>
// e <NumExchanges>
// f <filename> is chat file or file to bcast
// s <servername>
// p <portname>
// n <clientname>
// u <userid>
// S - flag indicating this is a server (no other switches than the UDP port applies)
// End of switches
#define LINUX_OPTS  "Shbm:e:f:s:p:n:u:v"
struct TOpts
{
    bool bBroadcaster;
    char cMasterSlave;
    int nExchanges;
    string strFilename;
    string strServerAddr;
    int nPort;
    string strClientName;
    int nUserID;
    bool bIsServer;
    bool bVerbose;
};

void InitOpts(TOpts *ptrOpts)
{
    ptrOpts->bBroadcaster = false;
    ptrOpts->cMasterSlave = 's';
    ptrOpts->nExchanges = 10;
    ptrOpts->nPort = 4097;
    ptrOpts->nUserID = 0;
    ptrOpts->strClientName = "ClientX";
    ptrOpts->strFilename = "none";
    ptrOpts->strServerAddr = "dev01";
    ptrOpts->bIsServer = false;
    ptrOpts->bVerbose = false;

}

int TestServer(TOpts *ptrOpts)
{
    int nRC = -1;
    if(ptrOpts->bVerbose)
    {
        cout << "This will execute a server on port: " << ptrOpts->nPort << endl;

    }
    nRC = 0;
    return nRC;
}

int TestClient(TOpts *ptrOpts)
{
    int nRC = -1;
    if( ptrOpts->bVerbose )
    {
        cout << "Client test data:" << endl;
        cout << "Client name: " << ptrOpts->strClientName.c_str() << endl;
        cout << "Client Number: " << ptrOpts->nUserID << endl;
        cout << "This client will contact: " << ptrOpts->strServerAddr.c_str() << " on port " << ptrOpts->nPort << endl;

        if( ptrOpts->cMasterSlave == 'm' )
        {
            cout << "This is a master client." << endl;
        }
        else
        {
            cout << "This is a slave client." << endl;
        }
        if( ptrOpts->bBroadcaster )
        {
            cout << "This is a broadcast test." << endl;
            cout << "The file to be broadcast is: " << ptrOpts->strFilename.c_str() << endl;
        }
        else
        {
            cout << "This is a chat test. The chat content will be sucking in from file: " << ptrOpts->strFilename.c_str() << endl;
            cout << "This chat test will consist of " << ptrOpts->nExchanges << " exchanges." << endl;
        }


    }

    return nRC;
}

void Usage()
{
    cout << "UDP Test tool 1.1" << endl
        << "Switches: " << endl
        << "-b - broadcaster" << endl
        << "-h - help" << endl
        << "-m <m | s> - master or slave" << endl
        << "-f <filename> - file to use for test" << endl
        << "-s <servername> - which server to connect to" << endl
        << "-p <port> - port to use" << endl
        << "-e <exhanges> - how many chat exhanges to do" << endl
        << "-n <clientname> - textual name of this client" << endl
        << "-u <UserNum> - numerical id of client" << endl
        << "-v - verbose" << endl
        << endl;

}

int main (int argc, char *argv[])
{
    int nResult = -1;
    TOpts Opts;
    InitOpts(&Opts);
    opterr = 0;
    int c;
    while((c = getopt(argc, argv, LINUX_OPTS)) != EOF)
    {
        switch(c)
        {
            case 'b':
                {
                    Opts.bBroadcaster = true;
                    break;
                }
            case 'v':
                {
                    Opts.bVerbose = true;
                    break;
                }
            case 'h':
                {
                    Usage();
                    return 0;
                }
            case 'm':
                {
                    Opts.cMasterSlave = optarg[0];
                    break;
                }
            case 'f':
                {
                    Opts.strFilename = optarg;
                    break;
                }
            case 's':
                {
                    Opts.strServerAddr = optarg;
                    break;
                }
            case 'p':
                {
                    Opts.nPort = atoi(optarg);
                    break;
                }
            case 'n':
                {
                    Opts.strClientName = optarg;
                    break;
                }
            case 'u':
                {
                    Opts.nUserID = atoi(optarg);
                    break;
                }
            case 'e':
                {
                    Opts.nExchanges = atoi(optarg);
                    break;
                }
            case 'S':
                {
                    Opts.bIsServer = true;
                    break;
                }
            case '?':
                {
                    // error
                    cout << "Unrecognized switch: -" << optopt << endl;
                    return -1;
                }
        }
    }

    // gets simple from here...
    if( Opts.bIsServer)
    {
        nResult = TestServer(&Opts);
    }
    else
    {
        nResult = TestClient(&Opts);
    }

    return(nResult);
}

This may be overkill in some situations but it does present an elegant way of deaing with switches...

I like it and is really easy to use once you get the hang of it....

[cas]

This is a fairly convoluted program, and there are a number of problems. Most basically, you're not using argc to check how many arguments to process. Rather, you just keep on converting argv elements until you find one that is less than 1 or greater than 9. What happens if the user doesn't enter an invalid entry? Well, you wind up calling atoi(NULL) which is invalid (NULL is the last entry in argv). You also seem to be trying to run the program with arguments that are larger than 9, but your do/while loop only accepts arguments between 1 and 9, inclusive. Is that what you want? In addition, after initializing args_main to zeroes, you forget to reset control_1 to zero, so instead of putting the converted argv elements into args_main[0], args_main[1], and so on, you place them beyond the end of the array (since control_1 is 128 after your initialization loop). Also, your use of args_main_p is not right. You don't actually use it for anything, but when you allocate memory with malloc(), you're overwriting its old value, which you appear to have set with a reason.

I'll show you an example of a cleaner way to do what it seems you're doing:

#include <stdlib.h>
#include <stdio.h>

#define MAX 128

int main(int argc, char **argv)
{
  int args_main[MAX] = { 0 }; /* This sets all elements to zero. */
  int i;

  for(i = 1; i < argc; i++)
  {
    /* The arguments in argv start at 1, so subtract 1 when storing
     * to args_main so that it starts at 0.
     */
    args_main[i - 1] = atoi(argv[i]);

    /* I'm not sure what your criteria are; this stops when a 0 or
     * negative argument is found.
     */
    if(args_main[i - 1] < 1) break;
  }

  /* I assume 0 is a terminating value. */
  for(i = 0; args_main[i] != 0; i++)
  {
    printf("%d ", args_main[i]);
  }
  putchar('\n');

  return 0;
}

There are problems with this code, such as the fact that args_main might overflow (if too many command-line arguments are given), and atoi() is not a safe function. But you get the idea, I hope.

[cas]

while((c = getopt(argc, argv, LINUX_OPTS)) != EOF)

If I may be ever so pedantic, getopt() returns -1 when it runs out of arguments, and EOF need not be -1. It probably is on most systems, but why risk it?

[London Fog]

Your assigning values of 'args_main' with indices 128 to who knows? You probably want to go from 0 to argc. Finally

I can't believed I missed that. Go me :P That fixed my problem. Thanks a bunch

[nadroj]

I can't believed I missed that. Go me :P That fixed my problem. Thanks a bunch

It happens. However, this is more likely to happen when you have a number of arrays, variables, counters, etc, as you have. I would try to reduce the number of variables you have, if possible, to reduce the probability of having another one of these logic errors.