Vigenere encryption assignment for CS50 C course My third week on cs50 and looking for some feedback on the general tidyness and style of my code. I took a look at other peoples code after I was done with this and I guess I could've implemented some functions(?) of my own but I noticed he used things that I haven't learned yet. 
E.g: Vigenere Cipher in C

Please criticize! :)

#include &lt;stdio.h&gt;
#include &lt;cs50.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;string.h&gt;
#include &lt;ctype.h&gt;

int i, c;
char* key;

int main(int argc, string argv[])
{
    int int_A = (int) 'A';
    int int_a = (int) 'a';


    // validate the command line argument's parameters
    if (argc != 2)
    {
        printf("Please use a single alphabetic command line argument\n");
        return 1;
    }
    else if (argc == 2)
    {
        for (int z = 0, n = strlen(argv[1]); z &lt; n; z++)
        {
            if (isalpha(argv[1][z]) == false)
            {
                printf("Please use a single alphabetic command line argument\n");
                return 1;
            }
        }
    }



    // get users plaintext
    printf("Plaintext: ");
    char* plaintext = get_string();


    // foo bar mitzva
        key = argv[1];
    int key_len = strlen(argv[1]);
    int key_i[key_len];
    for (i = 0; i &lt; key_len; i++)
    {
        if (isupper(key[i]))
        {
            key_i[i] = (argv[1][i] - int_A);
        }
        if (islower(key[i]))
        {
            key_i[i] = (argv[1][i] - int_a);
        }



    // set i back to 0 for the next loop
    i = 0;
    // iterate over characters in plaintext and shift plaintext character by key

    printf("ciphertext: ");
    for (int j = 0, n = strlen(plaintext); j &lt; n; j++)
    {
        int pt = (int) plaintext[j];
        if (isalpha(plaintext[j]))
        {
            if (isupper(plaintext[j]))
            {
                printf("%c", (((pt - int_A) + key_i[i % key_len]) % 26) + int_A);
            }
            if (islower(plaintext[j]))
            {
                printf("%c", (((pt - int_a) + key_i[i % key_len]) % 26) + int_a);
            }
        }
        else
        {
            printf("%c", plaintext[j]);
        }
        if (isalpha(plaintext[j]))
        {
            i += 1;
        }

    }
    printf("\n");
    return 0;
}
I'm a C++ programmer. I've never written a C program before. But most of what I say should be applicable.

Globals

int i, c;
char *key;


There's no reason for these variables to be global. These variables should be declared in the same place they are used. 

These variables also have very bad names. What is i? What is c? I'll have to read the rest of the code to find out.

String typedef

int main(int argc, string argv[])


I don't think using string is a good idea. Somewhere in that cs50 header there's probably

typedef char *string;


When you apply const to this typedef, you don't get what you expect. You may expect that const string is the same as const char *, but it isn't. const string is the same as char *const. You should use a pointer to a character when you want a pointer to a character.

Declare variables as late as possible

int int_A = (int) 'A';
int int_a = (int) 'a'; 


When you're declaring your variables, you should leave it to the last minute. If you don't need the variables yet, don't declare them.

Conditions

if (argc != 2)
{
    // The wrong number of arguments
}
else if (argc == 2)
{
    // The right number of arguments
}


If the condition argc != 2 is false, we know that the condition argc == 2 must be true so there is no need to confirm that it is true. Also, when there is the wrong number of arguments, the function returns. Your if should become this:

if (argc != 2) 
{
    // The wrong number of arguments
} 
// The right number of arguments


Unhelpful error messages

When the user doesn't provide exactly 1 argument, this happens:

printf("Please use a single alphabetic command line argument\n");
return 1;


When the user provides a single argument that does not consist of only alphabetic characters, this happens:

printf("Please use a single alphabetic command line argument\n");
return 1;


You should print two different error messages for two different problems. Perhaps "Please provide a single argument" and "Please provide an alphabetic string"

Bad variable names

for (int z = 0, n = strlen(argv[1]); z &lt; n; z++)
{
    if (isalpha(argv[1][z]) == false)
    {
        printf("Please use a single alphabetic command line argument\n");
        return 1;
    }
}


Here, you're referring to the key as argv[1]. At this point, it might make more sense to say "the key must be an alphabetic string" rather than "the first command-line argument must be an alphabetic string". You're also calculating the size of the key twice. You're doing it here in the for loop, then right after it

// foo bar mitzva
    key = argv[1];
int key_len = strlen(argv[1]);
int key_i[key_len];


We've found the place where key is used. You should put this snippet before the for loop and have the for loop refer to key rather than argv[1].

Strings are pointers, pointers can move

The part of your code that makes sure the key is an alphabetic string can be made more efficient. Your code finds the length of the key twice and then checks to see if every character is alphabetic. This is a whopping 3 iterations when only one is necessary.

char *key = argv[1];
for (const char *k = key; *k != 0; ++k)
{
    if (!isalpha(*k))
    {
        printf("The cipher key must be an alphabetic string\n");
        return 1;
    }
}


Is this a heap allocation?

// get users plaintext
printf("Plaintext: ");
char* plaintext = get_string();


I'm not sure if get_string allocates memory for the string or returns a pointer to static memory. You might want to specify that in a comment. If plaintext is heap allocated, you should call free(plaintext) to free the memory when you're done with it (at the very end of the program).

Characters are numbers

for (i = 0; i &lt; key_len; i++)
{
    if (isupper(key[i]))
    {
        key_i[i] = (argv[1][i] - int_A);
    }
    if (islower(key[i]))
    {
        key_i[i] = (argv[1][i] - int_a);
    }


We've finally found the place where that global i is used. You should declare variables right when you need them. I'm pretty sure you're missing a } at the end of this for loop. 

Here, you're copying the key into an array of ints. There is a bit of duplicate work here. If you know that a character is alphabetic, then it can only be upper case or lower case. If you know the character is upper case, then you know that it cannot be lower case. You could actually do this in the same place that you are checking you're characters are alphabetic. What I'm saying is:

char *key = argv[1];
const int key_len = strlen(key);
//If this was C++, I would definitely perform a heap allocation but
//heap allocation is a pain in C so I'll just use this C99 trick.
int key_i[key_len];
for (int idx = 0; idx != key_len; ++idx)
{
    const char character = key[idx];

    if (islower(character))
    {
        // characters are numbers, remember?
        key_i[idx] = character - 'a';
    } 
    else if (isupper(character))
    }
        key_i[idx] = character - 'A';
    }
    else 
    {
        //If the character is not lower case, and not upper case, then its not alphabetic
        printf("The cipher key must be an alphabetic string\n");
        return 1;
    }
}


The actual cipher part of the cipher program

Alright, now I'll find out what a Vigenere cipher is!

// set i back to 0 for the next loop
i = 0;
// iterate over characters in plaintext and shift plaintext character by key

printf("ciphertext: ");
for (int j = 0, n = strlen(plaintext); j &lt; n; j++)


Use proper variable names. What is i? What is j? You should rename them to key_idx and plaintext_idx respectivly.

Conditionals (again)

int pt = (int) plaintext[j];
if (isalpha(plaintext[j]))
{
    if (isupper(plaintext[j]))
    {
        printf("%c", (((pt - int_A) + key_i[i % key_len]) % 26) + int_A);
    }
    if (islower(plaintext[j]))
    {
        printf("%c", (((pt - int_a) + key_i[i % key_len]) % 26) + int_a);
    }
}
else
{
    printf("%c", plaintext[j]);
}
if (isalpha(plaintext[j]))
{
    i += 1;
}


pt never changes so it should be declared const. There's no need to repeatedly index the plaintext when you already have pt just lying around, not doing much. Try not to pack a whole load of math into one line. If you know that an alphabetic character is not upper case then it must be lower case. This snippet could be simplified to this:

const int pt = plaintext[plaintext_idx];
const int key_char = key_i[key_idx % key_len];
if (isupper(pt))
{
    // most of those brackets are unnecessary.
    printf("%c", (pt - 'A' + key_char) % 26 + 'A');
    key_idx += 1;
}
else if (islower(pt))
{
    printf("%c", (pt - 'a' + key_char) % 26 + 'a');
    key_idx += 1;
}
else
{
    printf("%c", pt);
}


Characters, aren't they weird

Fun fact about C, characters literals aren't actually chars, they're ints! In a lot of places when C expects a "character", what it actually expects is an int. isupper and islower expect ints. printf("%c", integer) expects an int. Just keep that in mind when handling strings in C.

Some more optimization

This program could probably be made much smaller, faster and neater (key_i can be removed) but it's 22:00 in Australia and I have school tomorrow so that's all for now.