Can't figure out why check50 sad faced these two

Question

Edit2: Latest code (working)

int main (int argc, string argv[])
{
    // Checking for proper command line input
    if (argc != 2)
    {
        printf("Usage: ./substituion key\n");
        return 1;
    }

    // Checking for 26 character input key
    if (strlen(argv[1]) != 26)
    {
        printf("Key must contain 26 characters.\n");
        return 1;
    }

    // Checking for alphabetic input
    for (int i = 0; i < 26; i++)
    {
        if (argv[1][i] >= 'a' && argv[1][i] <= 'z')
        {
            argv[1][i] = argv[1][i];
        }
        else if (argv[1][i] >= 'A' && argv[1][i] <= 'Z')
        {
            argv[1][i] = argv[1][i];
        }
        else
        {
            printf("Key must only contain alphabetic characters.\n");
            return 1;
        }
    }

    // Checking for repeated characters
    for (int i = 0; i < 26; i++)
    {
        for (int j = 25; j > i; j--)
        {
            if (argv[1][i] == argv[1][j])
            {
                printf("Key must not contain repeated characters.\n");
                return 1;
            }
        }
    }

    // Removing key's sensitiity
    for (int i = 0; i < 26; i++)
    {
        if (argv[1][i] >= 'A' && argv[1][i] <= 'Z')
        {
            argv[1][i] = argv[1][i];
        }
        else
        {
            argv[1][i] -= 32;
        }
    }


    // Getting plaintext input
    string message = get_string("plaintext: ");

    // Substitution of plaintext to key
    for (int i = 0, n = strlen(message); i < n; i++)
    {
        for (int j = 0; j < 26; j++)
        {
            if (message[i] == j + 'A')
            {
                message[i] = argv[1][j];
                j = 26;
            }
            else if (message [i] == j +'a')
            {
                message[i] = argv[1][j] + 32;
                j = 26;
            }
        }
    }

    // Ciphertext
    printf("ciphertext: %s\n", message);
    return 0;
}

[OUTDATED/ANSWERED] Edit: Here's my code.

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

int main (int command, string line[])
{
    // Checking for proper command line input
    if (command == 2)
    {
        // Checking for 26 character input key
        if (strlen(line[1]) == 26)
        {
            // Checking for alphabetic input
            for (int i = 0; i < 26; i++)
            {
                if (line[1][i] >= 'a' && line[1][i] <= 'z')
                {
                    line[1][i] = line[1][i];
                }
                else if (line[1][i] >= 'A' && line[1][i] <= 'Z')
                {
                    line[1][i] = line[1][i];
                }
                else
                {
                    printf("Key must only contain alphabetic characters.\n");
                    return 1;
                }
            }

            // Checking for repeated characters
            for (int i = 0; i < 26; i++)
            {
                for (int j = 25; j > i; j--)
                {
                    if (line[1][i] == line[1][j])
                    {
                        printf("Key must not contain repeated characters.\n");
                        return 1;
                    }
                }
            }

            // Getting plaintext input
            string message = get_string("plaintext: ");

            // Substitution of plaintext to key
            for (int i = 0, n = strlen(message); i < n; i++)
            {
                for (int j = 0; j < 26; j++)
                {
                    if (message[i] == j + 'A')
                    {
                        message[i] = line[1][j];
                        j = 26;
                    }
                    else if (message [i] == j +'a')
                    {
                        message[i] = line[1][j] + 32;
                        j = 26;
                    }
                }
            }

            printf("ciphertext: %s\n", message);
            return 0;
        }
        else
        {
            printf("Key must contain 26 characters.\n");
            return 1;
        }
    }
    else
    {
        printf("Usage: ./substituion key\n");
        return 1;
    }
}

Hi, I found a similar question asked here but I couldn't quiet get the answer provided. The answer said that the outputs must be the same, but the outputs are the same.

The three similar keys (YUKFRN...) used to test by check50 in substitution.c seems to be for checking the case sensitivity of the code. The ciphertext for these three similar keys must be the same then, and my code did output same ciphertexts for each of them. But why was it wrong somehow? Thanks in advance.

Answer 1

Sorry, but the output is not the same. Here's what happens when I ran the first failed test case:

~/go/test/substitution/ $ ./substitution yukfrnlbavmwzteogxhcipjsqd
plaintext: This is CS50
ciphertext: c  kh50

Clearly, there is an issue.

You should run a couple of tests, using the keys used in the two failing tests, and use this as the plaintext:

ABCDE FGHIJ KLMNO PQRST UVWXY Z abcde fghij klmno pqrst uvwxy z

When you run these tests, you'll notice something interesting. All of the uppercase plaintext chars encode correctly.

When the lowercase plaintext chars are encoded, they only encode correctly when the matching key char is an uppercase letter. If both the plaintext char and the matching key char are lowercase, the encoding fails (or an unprintable char is produced.)

Can you explain the reason for using 32 here?

     message[i] = line[1][j] + 32;

Now, some programming tips. First, a very common newbie problem. The code to do something should be compartmentalized into as small a block of code as possible. It should not be spread across wide swaths of a program. In this case, I'm talking about validating the input.

In this code, the code block tests whether the input - the key - is valid, is at the beginning of the program, but the action taken is near the end of the program. Worse, the actual code that does the bulk of the program is contained within this if/then/else code block. This is a bad practice. When code has to be debugged or modified later, likely by someone else, it's really easy to not understand how all those pieces work together, or even notice the structure.

The best practice is to test for invalidity of the input. If it's invalid, then end the program and be done with it. In this way, the input validation code is contained within a few lines and isn't intermixed with later code.

In this case, it would be structured something like this....

if (command != 2)
{
    //code to end program
}

if (some other condition)
{
    //code to end program
}

for (loop to check something else)

{
    //code to check something else and end program if check fails
}

// all checks completed, start main work of program now. 

// never have ending clauses related to input checks way down here!

In other words, the code in the two else clauses at the end of the program should be near the beginning, before the meat of the program starts. They should be in code blocks associated to if statements at the beginning, not else statements anywhere.

Next tip: The standard form of main is this:

int main (int argc, char* argv[])

argc and argv are the standard variable names used here by convention. You can use anything and it'll work, but these are the traditionally accepted standard and all programmers will immediately know what they are - argc is the argument count and argv is the argument string array or vector.

Now, let's look at this:

// Checking for alphabetic input for (int i = 0; i < 26; i++) { if (line[1][i] >= 'a' && line[1][i] <= 'z') { line[1][i] = line[1][i]; } else if (line[1][i] >= 'A' && line[1][i] <= 'Z') { line[1][i] = line[1][i]; } else { printf("Key must only contain alphabetic characters.\n"); return 1; }

This is a lot of inefficient code. First, there's this:

                line[1][i] = line[1][i];

This is a "do-nothing" line of code that wastes execution cycles. Further, since this does nothing, the if and else if lines associated are also do-nothing code. Combined, they say "Check something and if true, do nothing."

You need to start thinking like this: "Is this something that's probably been done a lot? Then there's probably a function that already does it. And, it probably has a name that sounds a lot like what I want to do." There are thousands of standard library functions. You need to (and will) start to learn them. Remember, Google is your friend.

In this case, there are two useful tools that will reduce the code to just a few lines, a function and an operator, isalpha and the not operator, "!" :

// Checking for non-alphabetic input
for (int i = 0; i < 26; i++)
{
    if ( !isalpha(line[1][i]) )
    {
        printf("Key must only contain alphabetic characters.\n");
        return 1;
    }
}

Just giving you some coding tips and best practices, and correcting bad habits before they start, not trying to be harsh at all! Happy coding! ;-)

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