Finished vigenere, but it fails to work as it should

Question

*UPDATE: Actually, I can't understand a thing now. Let's take an example from check50. Our program should encrypt "world, say hello!" with a keyword "baz". It expects "xoqmd, rby gflkp!" as a correct encryption. But, wait a second! If I do it manually on paper, I will get:

world, say hello!

bazba--zba-zbazb-

which means I should move "w" by 2 (the number of "b" in the alphabet), "o" by 1 (the number of "a" in the alphabet) and "r" by 26 (the number of "z" in the alphabet) etc. I should not, however pay any attention to coma, exclamation point and space, right? So on paper I will get an encrypted message of "yprne, scz hdmlq!" which somehow does not equal the supposedly correct answer of "xoqmd, rby gflkp!" Why? Also, my program gives me yet another variant, encrypting it as a "yprle, tay hemlo!" Why? Please help me understand all this.

Can you please point me to the mistakes? But if it is possible, don't just correct the code, give me a hint of what to think about.

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


int main (int argc, string argv[])
{
    // checking, if there is indeed 2 command-line argumets
if (argc != 2)
{
    printf("Don't joke with me, cowboy! I said enter the name of the programm AND one keyword ONLY. Try again!");
    return 1;
}

// making a string out of the 2nd command-line argument
string keyword = argv[1];

// checking, if the keyword given is alphabetcial only
for (int i = 0, l = strlen(keyword); i < l; i++)
{
    if (isalpha(keyword[i]))
    {

    }

    else
    {
        printf("Looks like you've entered an invalid keyword. Remeber, your keyword must contain letter only. Try again!");
        return 1;
    }
}

// getting the text the user wants to cipher
printf("Enter the text you want to cipher: \n");
string plaintext = GetString();

// storing the length of the keyword
int kwl = strlen(keyword);

// a loop, that will go through the plaintext char by char, ciphering it with a corresponding keyword value
for (int i = 0, j = 0, l = strlen(plaintext); i < l; i++, j++)
{
    // Firstly - checking, if the programm should wrap over to the beginning of the key word, if its length was reached. Secondly - checking, if the current char in the keyword is upper- or lowercase, and
    if (j > kwl)
    {
        j = j%kwl;
    }

    int k = keyword[j];
    int kalph;
    if (isupper(keyword[j]))
    {
        kalph = k - 64;
    }
    else if (islower(keyword[j]))
    {
        kalph = k - 96;
    }

    // once the programm knows, by what value it should shift the original char, it checks, whether the current original char is upper- or lower case, and shifts it.
    if (isalpha(plaintext[i]) && plaintext[i]!= ' ' && isupper(plaintext[i]))
    {
        int shiftalph = (plaintext[i] - 64) + kalph;
        if (shiftalph > 25)
        {
            shiftalph = shiftalph % 26;
        }
        int ascii = shiftalph + 64;
        printf("%c", ascii);
    }
    else if (isalpha(plaintext[i]) && plaintext[i]!= ' ' && islower(plaintext[i]))
    {
        int shiftalph = (plaintext[i] - 96) + kalph;
        if (shiftalph > 25)
        {
            shiftalph = shiftalph % 26;
        }
        int ascii = shiftalph + 96;
        printf("%c", ascii);
    }
    else
        printf("%c", plaintext[i]);
}

}

Answer 1

There are a few problems with the code that need to be dealt with. Much of it involves starting counts and indexes at 1 when they should start at 0.

First, the easy problem. The pset instructions aren't very clear about this. First, they say to prompt for input. Then, they say to do it "as shown below", which is without a prompt. So, to make check50 work, remove the printf with the prompt for the plain text line.

When transforming letters from their ASCII values to appropriate numbers that you can encode, you are subtracting 96 or 64. The problem with that is that the letters are transformed to numbers from 1 to 26 inclusive. They should be transformed to numbers from 0 to 25 inclusive. Among other issues, z will never encode correctly and anytime the modulo operation, %, is applied, it will incorrectly encode the number. Since you wanted hints, I'll let you figure out how to fix it.

Next, j is the index that controls the keyword letter selection. In the code, j is incremented on every pass through the loop, i.e., for every character in the plain text. The j keyword index should only be incremented when a letter is encoded.

As a side note, there is a fair amount of unnecessary and/or redundant code. For example, look at the following:

if (isalpha(plaintext[i]) && plaintext[i]!= ' ' && isupper(plaintext[i]))

Consider the three test clauses in that statement. If a character is a letter, isalpha() will return true. By definition, the letter can't possibly be a space, so if the first clause is true, the second clause must be true. If the first clause is false, the second clause doesn't matter because it will never be evaluated. The whole if statement will short circuit the analysis because the first clause is false and will return false for the entire statement.

Now, let's evaluate the third clause. If a letter is uppercase, this is true. Again, by definition, if a letter is uppercase, it is also an alpha. (This is even mentioned in the documentation for isupper().)

Putting it all together, the entire statement could be replaced with the following:

if (isupper(plaintext[i]))

This would simplify the code and make it much more understandable.

Next is the following:

    if (shiftalph > 25)
    {
        shiftalph = shiftalph % 26;
    }

The if statement is unnecessary. All that is needed is the line with the modulo operation. Think about it. If >25, then the modulo line needs to execute. BUT, if < 25, what would happen if the modulo op executes? Nothing. The original number would be returned. So, the surrounding if statement is not needed.

You should always review code to see if there's anything that's extraneous or unnecessary and can be removed.

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

Answer 2

Look at line 46 - do you really need that check?