Ggthjy

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 keywordn");

        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 keywordn");  

        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;

}

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 keywordn");

        return 1;

    }



    ...

    if (valid == 1)

    {

        ...

    }

    else

    {

        printf("Usage: ./vigenere keywordn");  

        return 1;

    }

}

and could be shortened to:

int main(int argc, string argv[])

{

    if (argc != 2 || check(argv[1]) != 0)

    {

        printf("Usage: ./vigenere keywordn");

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

}