pset2 (Hail, Caesar) - How to refine my code

Question

i'm new to this and CS50.

int main(int argc, string argv[]) { if (argc <= 1 || argc > 2) { printf("Open program with a key!!!\n"); return 1; }

int k = atoi(argv[1]);
if (k < 0)
{
    printf("Give me a positive Int!!!\n");
    return 1;
}

string msg = GetString();

for (int i = 0, s = strlen(msg); i < s; i++)
{
    if (isspace(msg[i]))
    {
        printf(" ");
    }
    else if (msg[i] == 33)
    {
        printf("%c", msg[i]);
    }
    else if (msg[i] == 44)
    {
        printf("%c", msg[i]);
    }

    int c[i];
    if (isalpha(msg[i]))
    {
        if (isupper(msg[i]))
        {
            c[i] = msg[i] - 'A';
            c[i] = (c[i] + k) % 26;
            c[i] = c[i] + 'A';

            printf("%c", toupper(c[i]));
        }
        else if (islower(msg[i]))
        {
            c[i] = msg[i] - 'a';
            c[i] = (c[i] + k) % 26;
            c[i] = c[i] + 'a';

            printf("%c", tolower(c[i]));
        }
    }
}

printf("\n");

return 0;

}

Can someone please tell me the more refined way I could of done it please, as I just want to know for the future.

Thanks

Answer 1

One thing that sticks out about this is that you have special cases for spaces, commas, and exclamation marks. While I haven't tested your code directly, it looks like it wouldn't print characters like . or '. You really should just check if the character is alphabetical, and print every other character unaltered.

When you create int c[i], you aren't just creating an int - you're creating a whole array of ints. In fact, this produces a bug: your array will have i elements, but using c[i] later on in the code will try to access the i+1th element. (Remember that if an array has a size of, say, 3, the only valid indices are 0, 1, and 2.) This could lead to a segfault, or you could end up inadvertently altering another variable in your program.

It also might be better to use a char rather than an int as a placeholder, although it shouldn't cause any problems.

It's redundant to check both isalpha and isupper/islower.

It shouldn't be necessary to use toupper and tolower, since adding 'A' and 'a' will only give you upper- and lower-case characters, respectively.

Answer 2

HINT: make this -

c[i] = (c[i] + k) % 26;

DRY.