I was doing an encryption exercise in CS50, and I got stuck There is this part: "Add to caesar.c, below main, a function called, e.g., only_digits that takes a string as an argument and returns true if that string contains only digits, 0 through 9, else it returns false. Be sure to add the function’s prototype above main as well".

And here's my code:

#include <cs50.h>
#include <stdio.h>
#include <ctype.h>
#include <string.h>

bool only_digits(string s);

int main(int argc, string argv[])
{
    // Make sure program was run with just one command-line argument

    argc = only_digits(argv[1]);
    if(argc != 2)
    {
        printf("Usage: ./caesar key\n");
        return 1;
    }
    return 0;

}

// make sure that the character in argv[1] is a digit

bool only_digits(string s)
{
    // int n = strlen(s);
    for (int i = 0, n = strlen(s); i < n; i  )
    {
        if(isdigit(s[i]))
        {
            return 0;
        }
    }
    printf("Usage: ./caesar key\n");
    return 1;
}

I decided to make a for loop, where I could check whether the characters are digits, but I need only to check the second character, not "caesar", and because of that, I presume, my code fails and comes with "Usage: ./caesar key\n" all the time, and if I write only "./caesar" - it comes with "Segmentation fault (core dumped)". Apparently my code is flawed, but I can't find the solution to it. I would appreciate any help

CodePudding user response：

Welcome to StackOverflow !

Lets note a few mistakes/misuse.

• You can do this but I discourage you to.. Leave the argc aside. I mean, do not reuse it for something else. Create a dedicated variable instead..

• Always check for validity of input argument(s) before processing them. In your case, when the user gives no input you still check for number. The makes you access invalid memory area and can trigger a segmentation fault.

• In the example code you provide, only_digits either returns 0 or 1. Thus, checking if(argc != 2) always gets satisfied.

• Your function only_digits is supposed to return a boolean => either true (or 1) or false (or 0). By convention, during your learning process, I advice you to stick to true and false.

• Your function only_digits has a negated logic. It will return true if it has non digit only.

The resulting code would look like this:

#include <stdio.h>
#include <ctype.h>
#include <string.h>

bool only_digits(string s);

int main(int argc, string argv[])
{
    /* Make sure there are enough arguments passed to your program. */
    if (argc < 2) {
        goto usage;
    }

    // Make sure program was run with just one command-line argument
    bool isvalid = only_digits(argv[1]);
    if(isvalid == 0) {
        goto usage;
    }

    // everything is good.
    return 0;

usage:
    printf("Usage: ./caesar key\n");
    return 1;
}

// make sure that the character in argv[1] is a digit

bool only_digits(string s)
{
    for (int i = 0; i < strlen(s); i  ) {
        /* Here, we want to break the loop upon the first non-digit
         * character. Thus the negation `!` */
        if(!isdigit(s[i])) {
            return false;
        }
    }

    return true;
}

CodePudding user response：

The OP code is trying to do too many things, and getting the order mixed up. For instance, the task of the function only_digits() is to test a string as being made up of only ASCII digits, not reporting how to execute the program.

Below uses less code. The characters of the passed parameter (if there is one) are checked. If there was a parameter, and there was 1 characters to check, and the check reaches the end of the string, the return value is true signifying all is good.

#include <cs50.h>
#include <stdio.h>
#include <ctype.h>
#include <string.h>

bool only_digits(string s);

int main(int argc, string argv[])
{
    if( only_digits( argv[1] ) == 0 )
    {
        printf("Usage: ./caesar key\n");
        return 1;
    }

    /* more code here */

    return 0;
}

bool only_digits(string s)
{
    int i = 0;
    while( s && isdigit( s[i] ) )
        i  ; // loop until end of string (if there is one)
    return s && i && s[i] == '\0';
}

One wants to write concise code that doesn't give room for bugs to hide.
This 'burns' a few extra (exceedingly cheap) machine cycles. Who cares?

EDIT
It's a little known fact that C guarantees that the "post-ultimate" value of argv[] will be NULL. (ie: "a.out foobar" sets argv[0] to point at "a.out", argv[1] to point at "foobar", and argv[2] to be NULL. One can use this information to one's advantage.