Vigenere encryption assignment for CS50 C course

Question

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 <stdio.h>
#include <cs50.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

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 < 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 < 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 < 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;
}

Answer 1

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 < 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 < 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 < 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.

Answer 2

First, @Kerndog73 did an execellent review and mostly I will try to address things he didn't.

You mention that perhaps you should have used functions, you are correct. Generally the main program is used to control the rest of the program. The main program should call functions that parse the commandline, functions that set up for the rest of the program, functions that perform the main logic of the program and functions that clean up whatever needs to be cleaned up.

Common Macros
The code already includes stdlib.h. The include file contains two macros that are widely used, EXIT_SUCCESS and EXIT_FAILURE. The code might be more readable of these two macros were used. It is possible that the values of EXIT_SUCCESS and EXIT_FAILURE may change from architecture to architecture but the macros are defined on every system.

int main(int argc, char *argv[])
{
    int exit_status = EXIT_SUCCESS;

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

    int exit_status = execute_program_logic(argv[1]);

    return exit_status;
}

Boolean Values
The original C specification didn't specify a boolean variable type, and conditions returned integer values. Many programmers defined constants TRUE and FALSE as 1 and 0. There has been the addition of stdbool.h in later versions of C that defines a boolean type and the values true and false. It's possible that cs50.h either includes stdbool.h or it defines true and false, but it would be better if stdbool was explicitly included in the code.

Global Variables
@Kerndog73 is correct that using global variables is a bad programming practice. It is very hard to correctly write code that uses global variables, and it is even harder to maintain. One may have to search thousands of lines of code in multiple files to find where a global variable is being changed. Global variables aren't needed at all when all of the code is in a single function.

Magic Numbers
The code contains the number 26 in two places, this is obviously the number of letters between a and z, but in many cases that wouldn't be as clear, and it is defintely less readable than ALPHABET_COUNT or SIZE_ATOZ.

#define SIZE_ATOZ        26

This number can also be derived by 'z' - 'a' which also might be more meaningful.

Use Common Library Functions When Possible
The code contains this line

    char* plaintext = get_string();

The cs50 header file may define get_string(), however, some students may have had problems using it. It might be better to use the common C I/O fgets() or getline(), these should be available on all systems. You can write your own get_string() using either one of these functions.

Initialize Variables on Separate Lines
It is more readable and easier to maintain if variables are initialized on separate lines. The code contains

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

If you are going to inialize n here, it would be better to put the initization above the loop. The code could be simpler by just using strlen in the condition portion:

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

Note: My compiler reports a warning because strlen returns unsigned long (size_t) rather than int.

Answer 3

A handful of small issues missed by other reviewers:

---

int int_A = (int) 'A';

'A' is already of type int, so the cast is pointless. Even if this were C++, char → int is a promotion, so wouldn't need a cast. We could (and should) show that we don't modify it:

const int int_A = 'A';

But really, int_A is no easier to read, and no more informative, than plain 'A' where you need it.

---

if (argc != 2)
{
    // (error message)
    return 1;
}
else if (argc == 2)
{
    // (more code)

The else if test is redundant. If we're still in the function, then we passed the first test:

if (argc != 2)
{
    // (error message)
    return 1;
}

// (more code)

Reducing the amount of nesting is a Good Thing, as it means less state for anyone reading the code to keep track of.

---

Another redundancy:

    if (isalpha(plaintext[j]))
    {
        // ...
    }
    else
    {
        printf("%c", plaintext[j]);
    }

    if (isalpha(plaintext[j]))
    {
        i += 1;
    }

Here, the i += 1 (or more idiomatically, ++i) could be moved into the first if:

    if (isalpha(plaintext[j]))
    {
        // ...
        ++i;
    }
    else
    {
        printf("%c", plaintext[j]);
    }

---

Alphabetic arithmetic isn't portable the way it's done here. You're probably using a system that encodes characters using (a superset of) ASCII, but C supports other encodings, notably EBCDIC, where letters are not all consecutive. If you don't want to deal with these codings, it's reasonable to require that letters are consecutive, but in that case, we ought to be explicit about the requirement.

Here's a test that may be some help:

#if  'Z' - 'A' != 25  ||  'z' - 'a' != 25
#error Unsuitable character encoding
#endif

But do note that the runtime character coding can be changed using setlocale(), so this isn't a guarantee that can be used for every program.

Also be aware that the results of isupper(), islower() and isalpha() depend on the selected locale, too. In particular, there's no guarantee that isalpha() necessarily implies isupper() || islower(), which this program depends on (though that is true for the default ("C") locale, so we're okay as long as we don't use setlocale()).

Sorry if this all sounds complicated; character coding is unfortunately a big and messy subject!

---

Finally, we don't need to read the whole input into memory before we start encoding. Since output only depends on the current character and the program state (key and number of characters read so far), we can read character by character like this:

int c;
while ((c = getchar()) != EOF) {
    if (isalpha(c)) {
        // c = encoded(c)
    }
    if (putchar(c) == EOF) {
        perror("putchar");
        return EXIT_FAILURE;
    }
}