CS50 Vigenere program

Question

I wrote a program for a problem in Harvard's CS50 course, Vigenere (week 2).

Here is a description of what the program needs to do:

Design and implement a program that encrypts messages using Vigenère’s cipher.

• Implement your program in a file called vigenere.c in a directory called vigenere.

• Your program must accept a single command-line argument: a keyword, k, composed entirely of alphabetical characters.

• If your program is executed without any command-line arguments, with more than one command-line argument, or with one command-line argument that contains any non-alphabetical character, your program should print an error (of your choice) and exit immediately, with main returning 1 (thereby signifying an error).

• Otherwise, your program must proceed to prompt the user for a string of plaintext, p, (as by a prompt for plaintext:) which it must then encrypt according to Vigenère’s cipher with k, ultimately printing the result (prepended with ciphertext: and ending with a newline) and exiting, with main returning 0.

• With respect to the characters in k, you must treat A and a as 0, B and b as 1, …​, and Z and z as 25.

• Your program must only apply Vigenère’s cipher to a character in p if that character is a letter. All other characters (numbers, symbols, spaces, punctuation marks, etc.) must be outputted unchanged. Moreover, if your code is about to apply the j^th character of k to the i^th character of p, but the latter proves to be a non-alphabetical character, you must wait to apply that j^th character of k to the next alphabetical character in p; you must not yet advance to the next character in k.

• Your program must preserve the case of each letter in p.

I received full credit for my answer, but I still want to clean up the code if possible. I am new to programming, so I enjoy seeing what changes more experienced programmers would make.

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

int check(string argv);
int shift(char c);

int main(int argc, string argv[])
{
    //check for valid key and save
    int valid = 0;
    if (argc == 2)
    {
        valid = check(argv[1]);        
    }
    else 
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }


    //begin encryption
    int key;
    int index = -1;
    int new_code;
    if (valid == 1)
    {
        //get plaintext to encrypt
        string plaintext = get_string("plaintext: "); 
        printf("ciphertext: ");

        //iterate through each char of plaintext
        for (int j = 0, l = strlen(plaintext); j < l; j++)
        { 
            if (isalpha(plaintext[j]))
            {
                //iterate through the keyword only when there is an alpha in the plaintext
                index++;
                key = shift(tolower(argv[1][index % strlen(argv[1])]));
                new_code = (int) plaintext[j] + key;  
                if ((int) tolower(plaintext[j]) + key > 122)
                {
                    new_code = new_code - 26;
                    printf("%c", (char) new_code); 
                }
                else 
                {
                    printf("%c", (char) new_code);
                }
            }
            else
            {
                printf("%c", (char) plaintext[j]);
            }
        }
        printf("\n");
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");  
        return 1;
    }
}

//create key
int shift(char c)
{
    int key = (int) c - 97;
    return key;
}

//decide if key is valid
int check (string argv)
{
    int val = 0;
    for (int i = 0, n = strlen(argv); i < n; i++)
    {
        if (isalpha(argv[i]))
        {
            val = 1;   
        }
        else
        {
            val = 0; 
            break;
        }
    }
    return val;
}

Answer 1

It's usually best to return as early as possible for invalid cases. This avoids unnecessary nesting and complexity.

e.g. the main() function has a lot of redundant code:

int main(int argc, string argv[])
{
    //check for valid key and save
    int valid = 0;
    if (argc == 2)
    {
        valid = check(argv[1]);        
    }
    else 
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }

    ...
    if (valid == 1)
    {
        ...
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");  
        return 1;
    }
}

and could be shortened to:

int main(int argc, string argv[])
{
    if (argc != 2 || check(argv[1]) != 0)
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }

    ...
}

The main loop of the program could be abstracted to a separate function (after checking for a valid key / printing usage on error).

---

Similarly check() could be written as:

int check (string argv)
{
    for (int i = 0, n = strlen(argv); i < n; i++)
    {
        if (!isalpha(argv[i]))
        {
            return 1;
        }
    }

    return 0;
}

---

check() is not an informative name for a function. It should at least be check_key, or is_all_alpha or something. shift() is a similarly cryptic name.

---

By using strlen, we are unnecessarily iterating the string twice (once to find the null at the end, then again to process it). We can simply run until we find a null character:

for (int j = 0; plaintext[j]; ++j)

---

Don't use magic numbers. What are 122, 26, or 97?

---

Variables should be declared as close to their point of usage as possible, and initialized with real values. (key, index, new_code).

---

It would be better to use another variable (e.g. keyword) as an alias, rather than using argv[1] throughout the code.

There is no need to strlen() the keyword for every character.

---

The main loop always ends up printing a char. The rest of the code just decides what the char will be. So perhaps it could be abstracted into a function. Also, since we are expected to treat both 'a' and 'A' the same, we can simplify things by only outputting letters in lowercase.

I'd be inclined to do something like this (not tested):

string key = argv[1];
int key_len = strlen(key);
int key_i = 0;
for (int j = 0; plaintext[j]; ++j)
{
    char p = plaintext[j];
    char c = !isalpha(p) ? p : encode_char(p, key[key_i++ % key_len]);
    printf("%c", c);
}

printf("\n");
...

char encode_char(char plaintext, char key)
{
    assert(isalpha(plaintext)); // #include <assert.h>

    int k = shift(tolower(key));
    int p = shift(tolower(plaintext));

    return unshift((p + k) % 26);
}

Answer 2

It appears that you built and tested this code on a system using ASCII or similar encoding. Codings other than ASCII will cause you problems:

• Encodings that are supersets of ASCII have more characters that match isalpha() (e.g. in Latin-1, à is an alphabetic character).
• Other encodings have different numeric values for a, A and the other values you've written as integer literals.
• Some encodings (notably EBCDIC) don't have contiguous alphabetic runs, so performing arithmetic on characters won't give the results you want.
• isalpha() and related functions have undefined behaviour when passed negative values (in this code, that can happen on systems where char is a signed type).