0

My code works properly for the Vigenere problem, but only if the key and the plaintext have the same number of letters in them. I think the problem is in the loops I'm trying to implement, but I'm not sure where to go from here.

Here's the code:

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

int main (int argc, string argv [])
{

    if (argc !=2)
    {
        printf ("Error. Instructions not followed.\n");
        return 1;
    }

    string key = argv[1];
    for (int i = 0, n = strlen (key); i < n; i++)
    {
        if (!isalpha(key[i]))
        {
            printf ("Non-alphabetical characters.\n");
            return 1;
        }
    }
    printf ("plaintext: ");
    string plaintext = get_string();
    printf ("%s\n", plaintext);
    printf("ciphertext:");
    for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v, k < z; g++, k++)
    {
        if (isalpha(plaintext[k]) && isalpha(key[g]))
        {
            if (isupper(plaintext[k]) && isupper(key[g]))
            {
                printf("%c", (((((plaintext[k])-65)+(key[g]-65))%26)+65));
            }
        }
    }
}

Any help would be appreciated. Thanks in advance.

2

Your for loop syntax is not likely doing what you think it is.

for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v, k < z; g++, k++)

Comma operators are sometimes used in the first part of a for loop, when you want two initialization steps to take place before the loop starts.

Comma operators are sometimes used in the last part of a for loop, when you want two counters incremented at the end of each loop iteration.

However, they are rarely used in the middle expression. The comma operator

evaluates its first operand and discards the result, and then evaluates the second operand and returns this value.

In your loop, it will test g < v, and then throw the result away... and test k < z. If k < z the loop will continue. I don't know how you got this code to compile in the CS50 IDE. If you use the default make command, you'd get the following error:

error: relational comparison result unused [-Werror,-Wunused-comparison]

which is the compiler's attempt to tell you that the g < v comparison is being discarded.

If you want to make two comparisons in a for loop, you need to use a compound comparison (like you did in the if loops):

for (int g = 0, k = 0, z = strlen (plaintext), v = strlen (key); g < v && k < z; g++, k++)

This STILL won't completely solve your problem, because now the loop will exit when you run out of characters in key. You should only be testing to make sure k is in bounds for plaintext. Instead of incrementing g every time through the loop, you should be incrementing it once every time you use one of the key's characters (and then using modulus to wrap it around if it runs out of characters). I would try starting that loop from scratch and try to use printf statements just to step through plaintext and key correctly before worrying about the actual encryption.

As an aside, for your own sanity while debugging, I would highly recommend against putting that many expressions into a for loop header. One of the dangers (and ironically... greatest strengths) of C is that the compiler will pretty much let you do almost anything without complaining too much. However, the more things that can go wrong on one line, the harder it is to discover where a mistake really is.

| improve this answer | |
  • I really can't figure this out. Maybe it's best that I walk away from programming, I don't think I quite have the brains for it. – nghanima Sep 15 '17 at 16:38
  • @nghanima Everyone has these problems when they first start programming. The best recommendation I can give you is to start small. Any program is built up of tiny pieces. Don't worry about the entire assignment, just start fresh with a clean file and an empty main function. Paste in only the amount of code you absolutely need. Forget the command-line arguments, forget the encryption. Just start with a plaintext string and a key string and work on a simple for loop that prints out the plaintext one character at a time. ... – robert_x44 Sep 15 '17 at 18:33
  • @nghanima Each time you print out this char, print out one char of key on the same line (and increment the index to this key so that it wraps around). Mess around until you get this working. And open up a new question with this simple code asking for help if it's not doing what you expect. Always take a step back and work on one piece at a time if you're having trouble - instead of just adding a few things and hoping for the best. Don't let a huge wall of code overwhelm you - we've all been there. – robert_x44 Sep 15 '17 at 18:33

You must log in to answer this question.

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