cs50x 2020 - pset2 - substitution - duplicate characters in key

Question

I keep getting an error around handling duplicate characters in key when checking my code for the substitution problem within pset2 of the cs50 course 2020. My code and further details are below - can anyone please help with this? Thanks

The error message it gives me is

:( handles duplicate characters in key
timed out while waiting for program to exit

When I check my code for duplicate characters it seems to work fine (printing Usage: ./substitution key and ending the program)

Code below

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

int main(int argc, string argv[])
{
// Check that only one argument submitted



if (argc == 2)
{

    // Check that key contains 26 characters
    int keylen = strlen(argv[1]);
    if (keylen == 26)
    {
        // Check that all characters are letters
        for (int i = 0; i < keylen; i++)
        {
            bool lettercheck = isalpha(argv[1][i]);
            if (lettercheck == true)
            {
                // THIS IS CAUSING ERROR - Check that no letters have been repeated - put all in lowercase to do so
                for (int n = 0; n < i; n++)
                {
                    char currentletter = argv[1][i];
                    char previousletter = argv[1][i - 1];
                    if (tolower(currentletter) == tolower(previousletter))
                    {
                        printf("Usage: ./substitution key\n");
                        return 1;
                    }
                }
            }
            else
            {
                printf("Usage: ./substitution key\n");
                return 1;
            }
        }
    }
    else
    {
        printf("Key must contain 26 characters.\n");
        return 1;
    }
}
else
{
    printf("Usage: ./substitution key\n");
    return 1;
}

// Get user input

string input = get_string("plaintext: ");

//Transform input using key
for(int i = 0; i < strlen(input); i++)
{
    char currentletter = input[i];
    int testlower = islower(currentletter);
    int testupper = isupper(currentletter);
    if (testupper > 0)
    {
        int j = input[i] - 65;
        input[i] = toupper(argv[1][j]);
    }
    else if (testlower > 0)
    {
        int j = input[i] - 97;
        input[i] = tolower(argv[1][j]);
    }
}

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

Edit: Figured out solution - problem was with the second for loop was iterating against i - 1 times instead of n times

Code should have been

    charpreviouslletter = argv[1][n]

instead of

     charpreviousletter = argv[1][i - 1]

    for (int n = 0; n < i; n++)
            {
                char currentletter = argv[1][i];
                char previousletter = argv[1]**[i - 1]**

score 0 · Accepted Answer · answered Aug 20 '20 at 14:08

In this loop-

 // THIS IS CAUSING ERROR - Check that no letters have been repeated - put all in lowercase to do so
for (int n = 0; n < i; n++)
{
    char currentletter = argv[1][i];
    char previousletter = argv[1][i - 1];
    if (tolower(currentletter) == tolower(previousletter))
    {
        printf("Usage: ./substitution key\n");
        return 1;
    }
}

you're comparing only the current character to the previous character. This doesn't work for strings like abcdefca

Notice how, c and a have duplicates - but they're not right next to their originals and hence your logic won't find these duplicates. Your logic will only work for duplicates that are next to each other such as aabcddef.

Instead, you need to take a note of which characters you've encountered whilst looping through. If you encounter a character that you have already encountered, you know there's a duplicate.

Thankfully, the key is only expected to contain all 26 characters of the alphabet without any duplicates. This means you can simply have an int array of 26 slots - each slot counts the number of appearances of the letter at that index. 0th index stands for 'a', 1st for 'b' and so on.

This way, you can very easily get the index of an alphabetic character using letter - 'a', where letter is the alphabetic character. So if the letter was a, you'd get 0, which is indeed the index of 'a'

Also, you have a nested loop while traversing the key, this nested loop also traverses through the key. Except it does it only up until a certain index, the index being the current index of the outer loop. This seems wasteful and weird. Why not simply loop through once, check if current character is an alphabetic letter and also check if this letter has been encountered before. That's all you have to do!

int letter_presence[26];
char upperletter;
string key = argv[1];
if (strlen(key) == KEY_LEN)
{
    for (int index = 0; index < KEY_LEN; index++)
    {
        if (!isalpha(key[index]))
        {
            // Wrong key - invalid character
            printf("Usage: ./substitution key\n");
            return 1;
        }
        if (letter_presence[tolower(key[index]) - 'a'] == 0)
        {
            // This letter has not been encountered before
            letter_presence[upperletter - 'A'] = 1;
        }
        else
        {
            // Wrong key - Duplicate letters
            return 1;
        }
    }
    // All good
}

cs50x 2020 - pset2 - substitution - duplicate characters in key

1 Answers1

Linked