cs50 pset2:substitution. My code is not working for the letters 'o' and 'z'

Question

So I am on week 2 solving the substitution assignment. I have already made the program and it works for the most part; however, it is not working for just one encryption key i.e:-

DWUSXNPQKEGCZFJBTLYROHIAVM

all the other keys that are used in the check50 are giving the required output except this one.

Input:-

The quick brown fox jumps over the lazy dog

and the Output:-

Rqx tokug wljif nja eozby ohxl rqx cdzv sjp

I just used the debug50 command and saw that the letters 'o' and 'z' completely skipped the alphabet check. Here's most of my code:

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


bool check_validity(string key, int key_length);
string encrypt(string key, string input);

char key_char[26];

int main(int argc, string argv[])
{
    int count = argc;
    // Exit Status if key is not given.
    if (count < 2 || count > 2)
    {
        printf("Usage: ./substitution key\n");
        return 1;
    }
    string key = argv[1];
    int key_length = strlen(key);


    bool valid = check_validity(key, key_length);

    // Exit Status if key is invalid.
    if (valid == false)
    {
        return 1;
    }



    // transfering the key in an array.
    for (int i = 0; i < 26; i++)
    {
        key_char[i] = tolower(key[i]);
    }


    string input = get_string("plaintext: ");

    string output = encrypt(key, input);

    printf("ciphertext: %s\n", output);


}


// Check validity.
bool check_validity(string key, int key_length)
{
// this function is working perfectly fine so removed it
}


string encrypt(string key, string input)
{
    string output = input;
    int index;

    for (int i = 0, n = strlen(input); i < n; i++)
    {
        // Check if the current char is an alphabet or not.
        if ((key[i] >= 65 && key[i] <= 90) || (key[i] >= 97 && key[i] <= 122))
        {
            // Check if the current char is lowercase or uppercase.
            if (islower(input[i]))
            {
                index = input[i] - 97;
                output[i] = tolower(key_char[index]);
            }
            else if (isupper(input[i]))
            {
                index = input[i] - 65;
                output[i] = toupper(key_char[index]);
            }
        }

    }
    return output;
}

In the encrypt function, the conditional under the for loop is used to check if the current character is a letter only, but for 'o' and 'z' this is simply skipped and I have no idea why. Can someone kindly explain?

Answer 1

The major problem lies in the condition:

if ((key[i] >= 65 && key[i] <= 90) || (key[i] >= 97 && key[i] <= 122))

Since you use the isupper() and islower() and toupper() and tolower() functions (macros), there is no reason not to use isalpha() either. But you're indexing through the key, not the input string. For long messages (more than 26 characters), you are scanning outside the key string. And all the characters in the key are letters — the validation ensures that.

Fixing that is 'easy':

if (isalpha(input[i]))

I then get told that key is not used in the encrypt() function, because you use key_char for the indexing, and that's a global variable. That can be fixed two ways:

1. Drop the key parameter to encrypt().
2. Make key_char into a local variable in main() and pass it to encrypt() as the key — and then change references to key_char in encrypt() to key.

Using global variables is generally something to avoid so I chose the second option.

There are some other cleanups — reporting errors on standard error, not standard output, using the actual program name in the usage message, using fewer variables in main(), etc.
It is puzzling that you use a random mix of using the functions/macros from <ctype.h> such as isupper(), islower(), tolower(), toupper() (but not isalpha()) along with uses of constants like 65, 90, 97, 122. Use the functions whenever possible; use 'A' and 'a' instead of 65 and 97 when relevant.

It is simpler to use if (count != 2) than to write out the equivalent if (count < 2 || count > 2). In a slightly different vein, your loop for (int i = 0, n = strlen(input); i < n; i++) could be simpler as for (int i = 0; input[i] != '\0'; i++).

There is a small risk of confusion since the input and output strings are actually the same string, the input string. It is mostly harmless, but you've destroyed the original by the encryption process. I've not fixed this.

The use of #include <assert.h> and the assertion in check_validity() — which might be better renamed is_valid_key() — ensure that the arguments are used even though it is a dummy function. That's necessary to get the code to compile with my default compilation options. I also made the code loop on standard input rather than stopping after reading one line.

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

bool check_validity(string key, int key_length);
string encrypt(string key, string input);

int main(int argc, string argv[])
{
    // Exit if key is not given or extra arguments are given
    if (argc != 2)
    {
        fprintf(stderr, "Usage: %s key\n", argv[0]);
        return 1;
    }
    string key = argv[1];

    // Exit Status if key is invalid.
    if (!check_validity(key, strlen(key)))
    {
        return 1;
    }

    // transfering the key in an array.
    char key_char[26];
    for (int i = 0; i < 26; i++)
    {
        key_char[i] = tolower(key[i]);
    }

    string input;
    while ((input = get_string("plaintext:  ")) != NULL)
    {
        string output = encrypt(key_char, input);
        printf("ciphertext: %s\n", output);
    }
    putchar('\n');

    return 0;
}

// Check validity.
bool check_validity(string key, int key_length)
{
    // this function is working perfectly fine so removed it
    // assert uses the otherwise unused arguments
    assert(key != 0 && key_length >= 0);
    return true;
}

string encrypt(string key, string input)
{
    string output = input;
    int index;

    for (int i = 0; input[i] != '\0'; i++)
    {
        // Check if the current char is an alphabet or not.
        //if ((key[i] >= 65 && key[i] <= 90) || (key[i] >= 97 && key[i] <= 122))
        if (isalpha(input[i]))
        {
            // Check if the current char is lowercase or uppercase.
            if (islower(input[i]))
            {
                index = input[i] - 'a';
                output[i] = tolower(key[index]);
            }
            else if (isupper(input[i]))
            {
                index = input[i] - 'A';
                output[i] = toupper(key[index]);
            }
        }
    }
    return output;
}

Example output

My terminal has echoctl set to echo control characters, hence the ^D. I called the program sc97 instead of signature, partly because I have a program of my own called signature which does a different job.

$ ./sc97 DWUSXNPQKEGCZFJBTLYROHIAVM
plaintext:  The quick brown fox jumps over the lazy dog.
ciphertext: Rqx tokug wljif nja eozby jhxl rqx cdmv sjp.
plaintext:  Pack my box with five dozen liquor jugs.
ciphertext: Bdug zv wja ikrq nkhx sjmxf cktojl eopy.
plaintext:  The five boxing wizards jump quickly.
ciphertext: Rqx nkhx wjakfp ikmdlsy eozb tokugcv.
plaintext:  How vexingly quick daft zebras jump.
ciphertext: Qji hxakfpcv tokug sdnr mxwldy eozb.
plaintext:  Bright vixens jump; dozy fowl quack.
ciphertext: Wlkpqr hkaxfy eozb; sjmv njic todug.
plaintext:  Glib jocks quiz nymph to vex dwarf.
ciphertext: Pckw ejugy tokm fvzbq rj hxa sidln.
plaintext:  ^D
$./sc97 DWUSXNPQKEGCZFJBTLYROHIAVM Antwerp
Usage: ./sc97 key
$ ./sc97
Usage: ./sc97 key
$ signature -dspl < src/data-files/alphabetic-phrases 
abcdefghijklmnopqrstuvwxyz  The quick brown fox jumps over the lazy dog.
abcdefghijklmnopqrstuvwxyz  Pack my box with five dozen liquor jugs.
abcdefghijklmnopqrstuvwxyz  The five boxing wizards jump quickly.
abcdefghijklmnopqrstuvwxyz  How vexingly quick daft zebras jump.
abcdefghijklmnopqrstuvwxyz  Bright vixens jump; dozy fowl quack.
abcdefghijklmnopqrstuvwxyz  Glib jocks quiz nymph to vex dwarf.
$