CS50 Vigenere Cipher

Question

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

int k = 0;
int convUpperAsciiToAlphaIndex = 65;
int convLowerAsciiToAlphaIndex = 97;
int AlphaIndexWrap = 26;

int main(int argc, string argv[])               // get Key from user via Command-Line Argument
{
                                                // printf("%i\n", i); test print to see value of i 
   if(argc != 2)
        {
            printf("Invalid command line argument (non-alphabetical character)\n");
            return 1;
        }
    string r = argv[1];
    int q = strlen(argv[1]);
    for(int i = 0; i < q; i++)    //iterate every char in string to see if isalpha
    {   
        if(isalpha(r[i]) == 0)
        {
            printf("Invalid command line argument\n");
            return 1;
        }
    }
    printf("plaintext: ");
    string plaintext = get_string();
    if (plaintext != NULL)
    {
        int c = strlen(plaintext);
        printf("ciphertext: ");
                                                    // printf("%i \n", c); test print strlen result
        for(int h = 0; h < c ; h++)
        {
            char d = plaintext[h];
            if(isalpha(d))
            {
                int m = (k % q);                    // Modulo wrap-around index tracking for Key
                int v = (r[m]);
                {
                    if((isupper(d) && isupper(v)))
                    {

                        // Converts Plaintext & Key from Upper ASCii to Alpha Index, ADDS the two resulting INTs (applies the Cipher), applies Modulo Wraparound for Plaintext Index
                        int x = (((d - convUpperAsciiToAlphaIndex) + (v - convUpperAsciiToAlphaIndex)) % AlphaIndexWrap);       

                        // converts back to ASCii
                        int w = (x + convUpperAsciiToAlphaIndex);                                                               
                        printf("%c", toupper(w));
                    }
                    else if((isupper(d) && islower(v)))
                    {
                        int toupper(int v);

                        // Converts Upper Plaintext & Lower Key from ASCii to Alpha Index, ADDS the two resulting INTs (applies the Cipher), applies Modulo Wraparound for Plaintext Index
                        int x = (((d - convUpperAsciiToAlphaIndex) + (v - convLowerAsciiToAlphaIndex)) % AlphaIndexWrap);      

                        // converts back to ASCii
                        int w = (x + convUpperAsciiToAlphaIndex);                   
                        printf("%c", toupper(w));
                    }
                    else if((islower(d) && isupper(v)))
                    {
                        int tolower(int v);

                        // Converts Lower Plaintext & Upper Key from ASCii to Alpha Index, ADDS the two resulting INTs (applies the Cipher), applies Modulo Wraparound for Plaintext Index
                        int x = (((d - convLowerAsciiToAlphaIndex) + (v - convUpperAsciiToAlphaIndex)) % AlphaIndexWrap);  // d - 97 + 2...converts to alpha index from lower and adds Key

                        // converts back to ASCii
                        int w = (x + convUpperAsciiToAlphaIndex);               // converts back to ASCII
                        printf("%c", tolower(w));
                    }
                    else if((islower(d) && islower(v)))
                    {

                        // Converts Plaintext & Key from Lower ASCii to Alpha Index, ADDS the two resulting INTs (applies the Cipher), applies Modulo Wraparound for Plaintext Index
                        int x = (((d - convLowerAsciiToAlphaIndex) + (v - convLowerAsciiToAlphaIndex)) % AlphaIndexWrap);  // d - 97 + 2...converts to alpha index from lower and adds Key

                        // converts back to ASCii
                        int w = (x + convUpperAsciiToAlphaIndex);               
                        printf("%c", tolower(w));
                    }
                    k++;
                }
            }
            else
            {
                printf("%c", (plaintext[h]));       // prints non-alpha chars
            }
        }
        printf("\n");
    }
    return 0;
}

Answer 1

none of the functions, etc available in math.h are being used in the posted code. It is a bad idea to include header files that are not being used.

NONE of the functions, etc in cs50.h are portable, Strongly suggest using the actual C language types and C library functions

when there is something wrong with the command line parameter list, the resulting error message should be output the stderr, not stdout and it should be a 'USAGE' message, similar to:

fprintf( stderr, "USAGE: %s <stringToEncode>\n", argv[0] ); 

the function: get_string() is not part of the cs50.h header and is not defined in the posted code. Perhaps you meant: GetString(). This shows that either you have not compiled the posted code or the posted code is not your actual code.

in general, it is highly more understandable if the code uses literals like 'a' or 'A' rather than the hardcoded values, And the 26 is a 'magic' number. magic numbers have no basis. Suggest using a enum statement or #define statement to give that 'magic' number a meaningful name then using the meaningful name throughout the code.

in the new code these lines:

int convUpperAsciiToAlphaIndex = 65;
int convLowerAsciiToAlphaIndex = 97;
int AlphaIndexWrap = 26;

just clutter the code and consume stack space.

Suggest replacing all occurrences of convUpperAsciiToAlphaIndex with 'A'.

Suggest replacing all occurrences of convLowerAsciiToAlphaIndex with 'a'.

Suggest replacing this line

int AlphaIndexWrap = 26;

with:

#define ALPHABET_SIZE 26

and replacing all occurrences of AlphaIndexWrap with ALPHABET_SIZE

variable names should indicate content or usage (or better, both).

This code:

   if(argc != 2)
    {
        printf("Invalid command line argument (non-alphabetical character)\n" );
    }

is not true and does not convey to the user what the problem is and does not output this error message to stderr. See my earlier comment about how to correct these code lines.

for ease of readability and understanding: separate code blocks (for, if, else, while, do...while, switch, case, default) via a single blank line.

this line:

printf("Invalid command line argument\n");

has problems:

1. fails to output the error message to stderr
2. fails to give the user any reason as to why the command line argument is not correct.

After the code expends lots of effort to validate the command line argument, then it gets a completely new string from stdin (I.E. the user) to actually encrypt. And the new string is not validated at all.