using atoi to convert command-line arguments to int, is only returning the first digit entered-CodePudding

I have to use command line arguments to set up a map for a snake game for my uni assignment. We were not specifically told to use atoi to help convert the command line argument from string to int, However I thought the simple nature of atoi would do the trick. On testing I discovered it is only taking the first digit.

int main(int argc, char *argv[])
{
    int isUserInput;
    char arg1, arg2, arg3;

    arg1 = argv[1][0];
    arg2 = argv[2][0];
    arg3 = argv[3][0];

    isUserInput = checkUserArg(arg1, arg2, arg3);

int checkUserArg(char arg1, char arg2, char arg3)
{
    int validation;
    int rowMap, colMap, snakeLength;

    rowMap = atoi(&arg1);
    colMap = atoi(&arg2);
    snakeLength = atoi(&arg3);

    if ((rowMap < 5) || (colMap < 5) || (snakeLength < 3))
    {
        validation = FALSE;
    }
    else if ((rowMap >= 5) || (colMap >= 5) || (snakeLength >= 3))
    {
        if (snakeLength < colMap)
        {
            validation = TRUE;
        }
        else
        {
            validation = FALSE;
        }
    }
    else
    {
        validation = FALSE;
    }
    
    return validation;
}

User has to enter 3 command line arguments (./file num1 num2 num3). I used atoi to convert the string command line arguments to int, but while testing I printed the numbers back and it won't convert the second digit only the first, e.g 1-9 works, but anything from 10 onwards only shows the first digit.

Any thoughts on why this is? Cheers.

CodePudding user response：

A single character is not a string. A string is an array of characters with null termination at the end.

You should do something like this instead:

bool checkUserArg (const char* arg1, const char* arg2, const char* arg3);

const since we shouldn't modify the args. Now this function can call atoi using the parameters directly.

However atoi is a broken function by design that should never be used, because it does not have well-defined error handling. Using it directly on argv is dangerous. You should always use strtol instead of atoi, it's a safer and more powerful function.

Example:

#include <stdlib.h>
#include <stdbool.h>

int main (int argc, char *argv[])
{
    if(argc != 3)
    {
      // error handling!
    }

    if(checkUserArg(argv[1], argv[2], argv[3]) == false)
    {
      /* error handling */
    }  
...

bool checkUserArg (const char* arg1, const char* arg2, const char* arg3)
{
   const char* endptr;
   ...
   rowMap = strtol(arg1, &endptr, 10); // 10 for decimal base
   if(endptr == arg1) // if these compare equal, the conversion failed
   {
     return false;
   }
   ...

   return true;
}

CodePudding user response：

Lets take this command for an example ./file 12 34 56, your code

arg1 = argv[1][0];
arg2 = argv[2][0];
arg3 = argv[3][0];

will give you 1, 3 and 5 respectively. Because you are storing the first character of each argument.

Here's the problem argv[1][0]

Some Improvements

TRUE and FALSE are not defined in C, you need to include stdbool.h
Don't use int as a boolean result, use bool from stdbool.h header file
TRUE and FALSE are defined in lower case in stdbool.h header file
Write like this int main(int argc, char const **argv) { }
There's no need to store argv values in another variable and then pass it to a function
Don't use atoi() function strtol() is more recommended
NOTE: char is only for single characters and const char *, char * and char [] are for storing multiple characters AKA strings.
Must check whether argv[n] even exists or not
Your function checkUserArg() should be have static linkage
Passing &arg1 to atoi() will cause Undefined Behavior
Must use return EXIT_SUCCESS;, which is defined in the header file stdlib.h, when exiting the application, or use return EXIT_FAILURE; if something goes wrong.
You should must check whether all arguments which are passed in your checkUserArg() function isn't NULL
Make sure that command line arguments contains only digits

Final Code

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

static bool is_numbers(const char *str)
{
    if(!str) // NULL
        return false;
    if(*str == 0) // ""
        return false;
    for (size_t i = 0; str[i]; i  )
        if (!isdigit(str[i]))
            return false;
    return true;
}

static bool checkUserArg(const char *arg1, const char *arg2, const char *arg3)
{
    if (!arg1 || !arg2 || !arg3)
        return false;
    bool validation;
    long rowMap, colMap, snakeLength;

    rowMap = strtol(arg1, NULL, 10);
    colMap = strtol(arg2, NULL, 10);
    snakeLength = strtol(arg3, NULL, 10);

    if ((rowMap < 5) || (colMap < 5) || (snakeLength < 3))
        validation = false;
    else if ((rowMap >= 5) || (colMap >= 5) || (snakeLength >= 3))
    {
        if (snakeLength < colMap)
            validation = true;
        else
            validation = false;
    }
    else
        validation = false;
    return validation;
}

int main(int argc, char const **argv)
{
    if (argc != 4)
    {
        fprintf(stderr, "Usage: %s <NUM> <NUM> <NUM>\n", argv[0]);
        return EXIT_FAILURE;
    }

    if (!is_numbers(argv[1]) || !is_numbers(argv[2]) || !is_numbers(argv[3]))
    {
        fprintf(stderr, "arguments should be only numbers\n");
        return EXIT_FAILURE;
    }

    bool isUserInput = checkUserArg(argv[1], argv[2], argv[3]);

    return EXIT_SUCCESS; // write at the end of the `main()` function
}

CodePudding user response：

There are multiple problems in your code:

atoi is only using the first digit because you explicitly extract the first digit and pass it as a char. The function call actually has undefined behavior as &arg1 is the address of a single char, not that of a null terminator C string.
checkUserArg converts the arguments using atoi which has undefined behavior if the value converted exceeds the range of type int. Using strtol is recommended and allows for finer checks.
checkUserArg should return the converted values to the caller via pointer arguments.
the second test in checkUserArg is redundant: if the first test is false, then all 3 comparisons in the second test will be true.
instead of TRUE and FALSE, you should use definitions from <stdbool.h>.

Here is modified version:

#include <errno.h>
#include <limits.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

bool convertArg(const char *arg, int *vp) {
    char *p;
    long num;

    errno = 0;
    num = strtol(arg, &p, 10);
    if (errno || p == arg || *p != '\0' || num < INT_MIN || num > INT_MAX) {
        *vp = 0;
        return false;
    } else {
        *vp = (int)num;
        return true;
    }
}

int main(int argc, char *argv[]) {
    int rowMap, colMap, snakeLength;

    if (argc != 4) {
        fprintf(stderr, "program needs 3 arguments\n");
        return 1;
    }
    if (!converArg(argv[1], &rowMap) || rowMap < 5) {
        fprintf(stderr, "invalid rowMap argument\n");
        return 1;
    }
    if (!converArg(argv[2], &colMap) || colMap < 5) {
        fprintf(stderr, "invalid colMap argument\n");
        return 1;
    }
    if (!converArg(argv[3], &snakeLength) || snakeLength < 3 || snakeLength >= colMap) {
        fprintf(stderr, "invalid snakeLength argument\n");
        return 1;
    }
    [...]
}