Vigenere Cipher in C

Question

I'm going through CS50 and one of the problem sets was to implement a vigenere cipher. I have had some previous classroom experience with C before CS50, but still consider myself a beginner.

The program takes in a command line argument as a key (and requires it to be composed of only alphabetical characters) and then prompts the user to input a string for plaintext (the string that needs to be encrypted). The key is mapped out such that A or a is 0, B or b is 1, C or c is 2, ...., Z or z is 25. The program applies the cipher to plaintext only if the character in plaintext is alphabetical. If any character in plaintext is non-alphabetical, it just prints it out in its original form. The case of each letter in plaintext is also preserved.

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


//this function checks if the command line argument has any non-alphabetic characters
int valid_command(string cmd);
//this function converts the key to an array of ints that correspond to each character in the key (ex: A or a = 0, B or b = 1, etc.) and returns a pointer to that array
int* convert_key (string cmd);
//calculate the length of the string
int length (string cmd);

int main (int argc, string argv[]){
    //cmd is used as a bool for the following if-statement- It's used to check if the command line argument (key) has any non-alphabetic characters
    int cmd = valid_command(argv[1]);

    //error checking for if the user input more than 2 command line arguments or if the key has any non-alphabetic characters
    if ((argc != 2) || cmd){
        printf("Error: Not valid input!\n");
        return 1;
    }
    else{
        printf("plaintext: ");
        string plaintext = get_string();
        int size = length(argv[1]);  //save the length of the key

        //convert key to an array of integers
        int* delta = convert_key(argv[1]);

        int* cipher = (int*)malloc((size+1)*sizeof(int));
        int* cipher_ptr = cipher;
        char* p_ptr = plaintext;

        //go through each character in plaintext, determine if it's an alphabetic character and whether it's in the A-Z range or a-z range and use a calculation based on that to figure out how the character should be encrypted.  This also takes into account wrapping of the key array (if the key array is smaller than plaintext, then once the last character in key is used to encrypt a character in plaintext, the next character should start over from the beginning of key) and also takes into account maintaining the case of the character of plaintext
        if (plaintext != NULL){
            int i = 0;
            while ((*p_ptr) != '\0'){
                //implement wrapping of key array
                if (i == size){
                    i = 0;
                }
                if (isalpha(*p_ptr)){
                    //if element is in A-Z range:
                    if (isupper(*p_ptr)){
                        *cipher_ptr = ((((*p_ptr)-65)+(delta[i]))%26) + 65;
                        p_ptr++;
                        cipher_ptr++;
                        i++;
                    }
                    else {
                        *cipher_ptr = ((((*p_ptr)-97)+(delta[i]))%26) + 97;
                         p_ptr++;
                        cipher_ptr++;
                        i++;
                    }
                }
                else {
                    //if the character in plaintext is non-alphabetic, just copy that into the cipher array, but don't go on to the next element in key array
                    *cipher_ptr = *p_ptr;
                    p_ptr++; 
                    cipher_ptr++;
                }
            }
        }
        else {
            return 1;
        }



        //print out the encrypted ciphertext
        printf("ciphertext: ");

        //put into chars
        char ciphertext [length(plaintext)];

        for (int i = 0; i < length(plaintext); i++){
            ciphertext[i] = *cipher;
            cipher++;
            printf("%c",ciphertext[i]);
        }
        printf("\n");

        return 0;
    }

}

//pass in the key string and return a pointer to array of integers that are integer representations of each character in key string
int* convert_key (string cmd){
    char* ptr = cmd;
    int* array = (int*) malloc(strlen(cmd)*sizeof(int));
    int* array_ptr = array;
    while ((*ptr) != '\0'){
        *array_ptr = (toupper((*ptr))-65);
        ptr++;
        array_ptr++;
    }
    return array;
}

//this function checks if the command line argument has any non-alphabetic character
//return 0 if there are no non-alphabetic characters
//return 1 if there are non-alphabetic characters
int valid_command(string cmd){
    char* ptr = cmd;
    if (ptr != NULL){
        while ((*ptr) != '\0'){
            int value = (toupper((*ptr))-65);
            if ((value > 25) || (value < 0)){
                return 1;
            }
            else {
                ptr++;
            }
        }
        return 0;
    }
    else{
        return 1;  //return 1 if no command was given
    }
}

int length (string cmd){
    int length = 0;
    char* ptr = cmd;
    while ((*ptr) != '\0'){
        length++;
        ptr++;
    }
    return length;
}

This works, but it seems a bit cumbersome, I think it could be shortened and made to be more readable. If you see any mistakes that my own testing and the grader didn't catch, I would greatly appreciate if you pointed that out as well. Any tips on improving my approach or logic would be awesome as well.

Answer 1

Structure

• Separate I/O from work. As a general pattern I suggest separating console input and output from work performing functions. This greatly helps testing and reuse.

• Functional decomposition. The problem seems to naturally decompose into a function that loops over the input and one that rotates individual characters.

• Considering that your range of math operation is basically 0-25 and you start and end with a char*, I do not see any particular advantage to using int for intermediate storage.

Syntax and Formatting

• Simplify conditions. Rather than two levels of conditionals:

  if (isalpha(...)) {
      if (isupper(...)) {
          ...
      } else {
          ...
      }
  } else {
      ....
  }
  

  This can be one level of conditionals:

  if (isupper(...)) {
      ....
  } else if (islower(...)) {
      ....
  } else {
      ....
  }
  

• Conciseness. The use of incrementing pointers in the original code seems to me more verbose and less clear than the equivalent for-loop.

• Clear, unambiguous use of types. The code freely intermixes and converts between string and char * suggesting they are the same. I do not see how string is adding any clarity here, particularly when defined hidden in an external include.

• Limit or eliminate magic numbers. I suggest you move the alphabet size to a constant such as ALPHABET_SIZE. Since 'A' and 'a' seem as clear as a constant and definitely clearer than their numeric value, I suggest describing the offsets using the ascii char.

• Comment formatting. My opinion is that the more regular and consistent the comments are the easier it is to scan for relevant information. I suggest a doxygen format. At a minimum, line-wrapped comments with standard spacing and formatting makes them easier to read and understand.

Allocation Handling

I suggest trying to be meticulous with tracking and releasing memory allocation as well as in documenting assumptions about NULL termination and pointer return values.

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

#define ALPHABET_SIZE 26

/**
 * Convert key from an alphabetic string to an array of alphabet offsets 0-25.
 * Ignores case.  (For example, A or a = 0, B or b = 1, etc.)
 * NOTE: Modifies the source string.
 *
 * @param[in] key - null terminated string representing alphabetic key.
 * @return pointer to modified string containing alphabet letter offsets 0-25
 */
char* convert_key (char *key){
    const int size = strlen(key);
    for (int ii = 0; ii < size; ++ii) {
        key[ii] = toupper(key[ii]) - 'A';
    }
    return key;
}

/**
 * Verify key is entirely alphabetic [a-zA-Z]
 *
 * @param[in] key -null terminated string representing Vigenere key
 * @return - 0 for TRUE all alphabetic, 1 for FALSE if NULL or non-alphabetic
 */
int is_valid_key(char *key){
    if (key == NULL) {
        return 1;
    }

    const int size = strlen(key);
    for (int ii = 0; ii < size; ++ii) {
        if (!isalpha(key[ii])) {
            return 1;
        }
    }
    return 0;
}


/**
 * Return a Vigenere rotation on a character if isalpha, 
 * else returns source unchanged
 *
 * @param[in] source - source character to rotate
 * @param[in] key - rotation value
 * @return enciphered character 
 */
char rotate(char source, char key) {
    if (isupper(source)) {
        return (((source - 'A') + key) % ALPHABET_SIZE) + 'A';
    } else if (islower(source)) {
        return (((source - 'a') + key) % ALPHABET_SIZE) + 'a';        
    } else {
        return source;
    }
}

/**
 * Encipher plaintext with key using Vigenere cipher.
 * Allocate and return ciphertext.
 *
 * @param[in] key - null terminated string containing alphabet letter offsets.
 * @param[in] plaintext - null terminated string containing plain text to encipher
 * @param[in,out] ptr_ciphertext - pointer to string which will be allocated and
 *                filled with enciphered text.  May return ptr to NULL on error.
 */
void encipher(char *key, char *plaintext, char **ptr_ciphertext) {
    assert(key != null);
    assert(plaintext != null);

    int txtlen = strlen(plaintext);
    int keylen = strlen(key);
    // This allocates and should be freed.  Arguably better to
    // simply malloc and clear. May be NULL on error
    *ptr_ciphertext = strdup(plaintext);
    if (*ptr_ciphertext) {
        for(int ii = 0; ii < txtlen; ++ii) {
            // Modulus keylen indexes into key repeatedly over range [0, keylen)
            *ptr_ciphertext[ii] = rotate(plaintext[ii], key[ii % keylen]);
        }
    }
}

int main (int argc, char **argv) {
    // Validity checks.

    // Verify count before dereferencing argv.
    if (argc != 2 || !is_valid_key(argv[1])) {
        printf("Error: Not valid input!\n");
        return 1;        
    }

    char *plaintext = get_string();
    if (plaintext == NULL) {
        return 1;
    }

    // The key from argv can be safely modified in place to form the key   
    char *key = convert_key(argv[1]);
    // While could save an allocation by modifying the source string directly
    // it may clearer to allocate a separate cipher text.
    char *ciphertext = NULL;  // Unnecessary but makes intention clear
    encipher(key, plaintext, &ciphertext);

    int retval = 1;  // Initially assume failure.
    if (ciphertext != NULL) {
        printf("ciphertext: %s\n", ciphertext);
        retval = 0;  // Report success
    }

    // cleanup allocated storage
    free(plaintext);
    free(ciphertext);
    return retval;
}