0

I've tried to figure out how an iteration could repeat itself. Iterations 1 and 4 seem to repeat. I've left in my debugging printf lines so you can see how these iterations in my nested FOR loops are repeating and changing the output from correct to incorrect.

With my limited knowledge, I don't understand how this is possible, so I'm having a hard time figuring out how to fix it.

Any thoughts or suggestions are very welcome.

Thank you!

Here is the output I'm getting:

substitution/ $ ./subtest YTNSHKVEFXRBAUQZCLWDMIPGJO

plaintext: HELLO

i: 0 text: H key: E new text: E

i: 1 text: E key: H new text: H

i: 1 text: H key: E new text: E

i: 2 text: L key: B new text: B

i: 3 text: L key: B new text: B

i: 4 text: O key: Q new text: Q

i: 4 text: Q key: C new text: C

ciphertext: EEBBC

... ...

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

string key;
string phrase;

string substitute();
string phrase_input();

int main(int argc, string argv[])
{
    key = argv[1];
    string key_two = key;

    if (argc != 2)
    {
        printf("Requires single argument.\n");
        return 1;
    }
    else if (strlen(key) != 26)
    {
        printf("Argument not valid0.\n");
        printf("%li\n", strlen(key));
        printf("%s\n", key);
        return 1;
    }

    for (int i = 0; i <= 26; i++)
    {
        if (isdigit(key[i]))
        {
            printf("Argument not valid1.\n");
            return 1;
            break;
        }
        else if (ispunct(key[i]))
        {
            printf("Argument not valid2.\n");
            return 1;
            break;
        }

        for (int j = 0; j < 26; j++)
        {
            int counter = 0;
            if (key[i] == key_two[j])
            {
                counter++;
            }
                if (counter > 1)
                {
                    printf("Argument not valid3.\n");
                    return 1;
                    break;
                }
        }
    }

    phrase = get_string("plaintext: ");

    string text = substitute(phrase);

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

    return 0;
}

string substitute(string text)
{
    int n = strlen(text);
    string alphabet_lower = "abcdefghijklmnopqrstuvwxyz";
    string alphabet_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

    for (int i = 0; i < n; i++)
    {
        for (int j = 0; j < 26; j++)
        {
            if (text[i] == alphabet_lower[j])
            {
                text[i] = tolower(key[j]);
            }
            if (text[i] == alphabet_upper[j])
            {

// debugging
                printf("i: %i\n", i);
                printf("text: %c\n", text[i]);

                text[i] = toupper(key[j]);

// debugging
                printf("key: %c\n", key[j]);
                printf("new text: %c\n", text[i]);
            }
            else if (ispunct(text[i]))
            {
                continue;
            }
        }
    }
    return text;
}

2 Answers 2

1

It's a logic error - something is missing. ;-)

The reason that it generates multiple prints for certain letters is hidden in the current logic.

Think about what happens. The inner loop runs through the entire alphabet. Once it finds the matching letter in the plain alphabet, it substitutes the key letter in the original text. But what happens next???? The inner loop continues to run and see if there's a match, instead of terminating the inner loop.

The output shows exactly what happens. Let's use the #4 case since it shows this more clearly than #1.

i: 4 text: O key: Q new text: Q
i: 4 text: Q key: C new text: C

In this case, the letter is "O". The loop runs until it finds O in the alphabet string. Then, it replaces O with Q. Now the fun starts. Since the inner loop doesn't terminate, it continues to run. BUT, now it's looking for Q, not O. Two passes later, it matches up and substitutes C for Q.

If the inner loop were terminated immediately after O is substituted for Q, then this problem goes away.

There are other issues, such as the first loop running on 26 vs. 25, trying to do things with argv[1] before checking the value of argc, and maybe some other stuff.

Programming tips:

Dead code: the break commands in main are what is called dead code. They will never be executed because they immediately follow return statements. It's impossible for code execution to ever get to them. The break commands can be completely eliminated from the code. Maybe you can use them somewhere else. ;-)

What happens if a character in the key is neither a digit, a letter or a punctuation mark? There are other possibilities. Instead of having multiple checks for every type of character, why not simplify it to "if not a letter"?

if (!isalpha(...) ) 

In the check for duplicated characters, why is key_two needed? Why not just compare key[i] to key[j]?

Just a few thoughts... ;-)

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)

1
  • Thank you! I was so stumped thinking the iterations were duplicated I didn't consider they were continuing and needed to be stopped. I did try the not isapha but I think I did it wrong and then forgot to figure it out before moving on to a different idea. Oops! Thanks again! and for the quick reply.
    – fawkeskc
    Commented Feb 26, 2023 at 6:04
1

One problem is: after a letter is substituted, and text[i] is changed, the j loop continues and might substitute again, depending on the key. After substitution program should break out of the j loop. NB: the continue (as in the ispunct test) will continue the j loop.

1
  • Thank you! My mind couldn't get past the iterations duplicating themselves instead of just continuing. Thanks again and for the quick reply!
    – fawkeskc
    Commented Feb 26, 2023 at 6:07

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .