I am getting different test results for the same code

Question

I am testing my substitution.c program, but I get every time different results for the same code.

On the very 1.st time I got all good but :( does not encrypt non-alphabetical characters using DWUSXNPQKEGCZFJBTLYROHIAVM as key expected "ciphertext: Yq...", not "". According assignment, non-alphabetical characters should stay unchanged.

So I tried another test with the same code and I got:

:) substitution.c exists
:) substitution.c compiles
:( encrypts "A" as "Z" using ZYXWVUTSRQPONMLKJIHGFEDCBA as key
    expected "ciphertext: Z\...", not ""
:) encrypts "a" as "z" using ZYXWVUTSRQPONMLKJIHGFEDCBA as key
:) encrypts "ABC" as "NJQ" using NJQSUYBRXMOPFTHZVAWCGILKED as key
:) encrypts "XyZ" as "KeD" using NJQSUYBRXMOPFTHZVAWCGILKED as key
:( encrypts "This is CS50" as "Cbah ah KH50" using YUKFRNLBAVMWZTEOGXHCIPJSQD as key
    expected "ciphertext: Cb...", not ""
:) encrypts "This is CS50" as "Cbah ah KH50" using yukfrnlbavmwzteogxhcipjsqd as key
:) encrypts "This is CS50" as "Cbah ah KH50" using YUKFRNLBAVMWZteogxhcipjsqd as key
:) encrypts all alphabetic characters using DWUSXNPQKEGCZFJBTLYROHIAVM as key
:( does not encrypt non-alphabetical characters using DWUSXNPQKEGCZFJBTLYROHIAVM as key
    expected "ciphertext: Yq...", not ""
:) handles lack of key
:) handles too many arguments
:) handles invalid key length
:) handles invalid characters in key
:) handles duplicate characters in uppercase key
:) handles duplicate characters in lowercase key
:) handles multiple duplicate characters in key

Once I've got all checks good, another time completely different. And apart this problem, the A character should be encrypted as Z with ZYXWVUTSRQPONMLKJIHGFEDCBA key. So why is the result negative? Any idea what could get wrong? Thanks!

My code:

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

// prototypes
int verifyKey(int argc, string key);
int encrypt(string key, string plaintext);
string toupperText(string text);

int main(int argc, string argv[])
{
    string key = argv[1];
    // verify used key
    int keyOK = verifyKey(argc, key);
    if (keyOK == 1)
    {
        return 1;
    }

    // prompt user for text to be encrypted
    string plaintext = get_string("plaintext: ");
    // print
    printf("ciphertext: ");
    // encrypt text
    int ciphertext = encrypt(key, plaintext);

    return 0;
}

int verifyKey(int argc, string key)
{
    const string WARNING_USAGE = "Usage: ./substitution key";

    // prompt user for exactly one cmd argument
    if (argc != 2)
    {
        printf("%s\n", WARNING_USAGE);
        return 1;
    }

    // cmd argument must be 26 characters long
    if (strlen(key) != 26)
    {
        printf("Key must contain 26 characters.\n");
        return 1;
    }

    // handle invalid characters in key
    for (int i = 0; i < strlen(key); i++)
    {
        if (!isalpha(key[i]))
        {
            printf("%s\n", "Key must contain only alphabetical characters.\n");
            return 1;
        }
    }

    // handle duplicate characters in key
    char testKey[strlen(key)];
    for (int i = 0; i < strlen(key); i++)
    {
        //
        for (int j = 0; j < strlen(testKey); j++)
        {
            if (key[i] == testKey[j])
            {
                printf("%s\n", WARNING_USAGE);
                return 1;
            }
        }
        testKey[i] = key[i];
    }

    return 0;
}

int encrypt(string key, string plaintext)
{
    // key to upper case for easier computing
    key = toupperText(key);
    // ASCII values from key
    int keyIndexes[strlen(key)];
    for (int i = 0; i < strlen(key); i++)
    {
        // get ASCII value for i-th key character
        int ascii = (int) key[i];
        keyIndexes[i] = ascii;
    }

    // array of ASCII values of encrypted text
    int encryptedtext[strlen(plaintext)];

    // loop through plaintext
    for (int i = 0; i < strlen(plaintext); i++)
    {
        char c = plaintext[i];
        // if character is alphabetical -> encrypt
        if (isalpha(c))
        {
            // get ASCII value of character
            int index = (int) c;
            // get position in alphabet
            index -= 65;
            int moveToUpper = 0;
            if (islower(c))
            {
                // get position in alphabet for lowercase characters
                moveToUpper = 32;
            }
            encryptedtext[i] = keyIndexes[index - moveToUpper] + moveToUpper;
        }
        else
        {
            encryptedtext[i] = c;
        }
        printf("%c", (char) encryptedtext[i]);
    }
    printf("\n");
    return 0;
}

string toupperText(string text)
{
    for (int i = 0; i < strlen(text); i++)
    {
        text[i] = toupper(text[i]);
    }
    return text;
}

Answer 1

First I want to commend you on good code structure. You broke it into appropriate functions, that properly dividing the logical into clear seperate pieces with singular responsibility. This makes reviewing your code much much easier!

Minor Issues

1. I would highly suggest that you either don't hide the checks of argc inside verifyKey unless you are going to return a pointer to a valid key instead of a boolean denoting success or pass both argc and argv to the function and consider renaming it verifyInput.
   IMHO This makes for clearer code, to me it is very, very, disconcerting to see you take a pointer to argv[1] in main when you haven't yet verified its existence. On the other hand I like that you combined all of the input verification together. Which is why I would suggest that you not return a bool, but instead return argv[1] if it was valid and in main instead check that key was non-null, and return NULL if it wasn't valid.

   i.e.

   // get a verified key or nothing
   string key = GetVerifiedKey(argc, argv); // modify name, signature, and 
   return value
   if (key == NULL)
   {
       return 1;
   }
   ...
   

   Now it's clear to a reader your function is going to give you a verified key back or nothing, and that it is going to likely handle printing of errors inside, and we aren't assigning non-sense values to unneeded variables. Minor style thing but I think it reads cleaner. I won't even touch on constness of argv or of the string types in here because the usage of string as the course teaches you to do, obscured its nature as a pointer which complicates constness which I don't think has been taught to you yet.

2. Minor issue but worth covering, especially since you are on the 'more' track. Inside verifyKey, calculate strlen once, and reuse its value. strlen is not cheap to calculate (ask yourself how much work the CPU has to do to figure it out). Granted in this case you know it will be unavoidable the first time and pretty reasonable (only 26) when you have to do it twice, but there's no need to run it twice, and worse you will ALWAYS calculate it twice on the "happy path". In general, you want to avoid repeating yourself. (DRY Don't Repeat Yourself). Capture the value of strlen once and then reuse it in the function as needed.

3. toUpperText has no reason to return anything. You are modifying the string directly. I would also consider doing the toUpperText out in main but this is very subjective, and would depend more on future thoughts of usage.

4. You set variable ciphertext equal to an int in main, but you never check its value. This indicates a bug; either check it or make it void.

5. It's also unclear why you don't print the ciphertext out in main, but print it's "heading" out in pain. I would suggest printing them together wherever you print them from. In this case since you haven't learned about dynamic memory yet, the logical thing to do is realize plaintext will have become modified into ciphertext, unless you are making a copy. I would however suggest making the copy outside in main and passing the to-be-modified string to encrypt and then do all the printing from main. I'd only make encrypt return something if there was a condition in which it needed to indicate an error.

6. In encrypt you do the strlen thing again except here it is a bit sillier, you know the key has to be of length 26 so why take strlen of key at all in here? And then avoid doing strlen twice for plaintext, except for plaintext I would suggest using

Now onto the real fun with your encrypt function

Non-Critical Issues in encrypt but worth examining

1. There is absolutely no reason for you to make keyIndexes. The string plaintext which is input to the function is actually a char * That is it is an array of char terminated by a NULL. You don't have to recreate an entire array of the ascii values because they are already there. So I would get rid of that entirely.

   1. More importantly I would first examine why you felt you needed to make it an array of int instead of an array of char. char doesn't have to be letters, char is JUST a smaller data type than int that is 1 Byte, so its range is from -256 to 255. ASCII is just interpolating those numeric values into "letters" when printing as "characters".

2. Do not use 'magic' numbers. 65 and 32 are magic numbers; someone reading the code doesn't know what or why they are what they are and has to verify what they are doing... Even knowing exactly what you are doing I discourage their direct use strongly. There is no need for it and gets you into a bad habit. The other problem is that you don't even need these.

   1. What is 65? 65 is just 'A's integral value which is already what the literal is so you could store this by saying char value_A = 'A'
      1. So you have no use for this const, you could replace it's usage by -= 'A' in your code currently and it would be more readable.
   2. As to 32, that is just char TOUPPER = 'a' - 'A'
      1. So again not really needed.
   3. C provides a tolower and toupper function. You used toupper. So why not just use islower and or isupper as well?

3. While I find your logic overly complex and confusing there is nothing inherently wrong with this approach. I find it helps to explain what code is doing or trying to do before fixing or writing it.

   In this case you iterate over each letter letter c in the plaintext, if it is not alphabetic you copy its value straight into encryptedText[i] correctly (Though its not entirely clear why) and then you print the single character. ( this is why I don't understand why you kept that as an array). If it is alphabetic, you want to find it's corresponding replacement in the keyIndex (which is set such that the 0th index is for 'A'/'a' etc), and then recapitalize as needed since the replacement value is always uppercase, and then you need to set it into encryptedText[i] and print that character (again doesn't seem to require an array).

   To handle uppercase and lowercase, you use a mixture of approaches which I find confusing and unneeded but they appear correct.

   If the letter was uppercase to start with, you subtract 65 from it and set index to that difference which will be in the keyIndex array as is, islower(c) returns false, and moveToUpper stays 0, and it works fine.

   If the letter was lowercase however its a bit more complicated. Let's say you had 'a', well its value is 97, 97-65 is 32, you set index to a value that is not in the keyIndex array but you do handle this later. So islower('a') is true, so moveToUpper is set to 32. Finally you have keyIndexes[index - moveToUpper] + moveToUpper] which is keyIndexes[32 - 32] + 32 which is keyIndexes[0]+32 which will give you 'A''s replacement with 32 added to it getting you lower case again. This should work...

   Now if it were me, I would have just done the following for your isalpha(c) is true branch:

   bool is_lower = islower(c);
   int offset = 0;
   int index = c; // I would have kept it as a char but that's a separate issue
   if ( is_lower ) {
       index -= 'a';
       offset = 32; // 'a'-'A'
   } 
   else 
   {
       index -= 'A';
   }
   encryptedText[i] = keyIndex[index] + offset; // even simpler replace keyIndex with key[index]
   // or dont use offset at ALL and add an extra line
   // if (is_lower) encryptedText[i] = tolower(encryptedText[i]);
   // this way we can consistently use alpha-functions all the time or none of the time.
   // in either case it lends itself to printing and debugging more easily you can check the values always come up to what they should
   

   It's the same code as yours but I think it reads much cleaner

As I haven't identified whats causing the failure I'll try it later tonight when I have access to the cs50 site from home.

What I would do in the interim

You know exactly what cases don't work so I would try those 2 cases locally in your IDE. The 'A' to 'Z' one and the other one that failed first ( ignore the non-alphabetical thing for the moment ).

Consider adding print statments inside your encrypt. (not for submission just for local testing)

• print out the character from plaintext being encrypted
• print out whether it was upper or lowercase
• print out the index and move to value before the assignment into encryptedtext.
• You should be able to figure out where it is dying.