CS50 pset2 Vigenere code - Outputs one incorrect letter

Question

New to this site and to programming. I've looked through the previous questions under this topic and tried any number of fixes, but I keep having the same problem.

My program runs fine and gives me the output I expect, with the exception of the letter 'B' or 'b'. Every other letter encrypts as it should. Where did I go wrong?

EDIT - When I encrypt the message, "Meet me at the park" with a key of "bacon", I should get: Negh zf av huf pcfx. Instead I get: Tegh zf av huf pcfx

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

int main(int argc, string argv[])
{
    //Get key in commandline argument.  Prompt until key is given.
    string key = argv[1];
    int key_length = strlen(key);

    if (argc != 2)
    {
        printf("Invalid command. Please specify key.");
        return 1;
    }
    //Make sure only alphabetical chars are used in key.
    for (int i = 0; i < key_length; i++)
    {
        if (!isalpha(key[i]))
        {
        printf("Invalid command. Please specify key.");
        return 1;
        }
    }

    //Get message to be encrypted
    string plain = GetString();

    for (int i = 0, j = 0; i < strlen(plain); i++)
    {
        if (isalpha(plain[i]))
        {
            if (isupper(plain[i]))
            {
                plain[i] = (((plain[i] - 65) + (key[j%key_length] - 65)) % 26) + 65;
                j++;
            }
            else
            {
                if (islower(plain[i]))
                {
                plain[i] = (((plain[i] - 97) + (key[j%key_length] - 97)) % 26) + 97;
                j++;
                }
            }
        }
    }
    printf("%s\n", plain);
    return 0;
}

Not related to your problem, but you should enforce that `argc == 2` before accessing `argv[1]`. Otherwise you might take the `strlen` of a null (or other invalid) string. — M Oehm, Jul 31 '15 at 18:09
Thanks for the help everyone! Turns out I misidentified the problem. My code wasn't responding well to the uppercase letters. I think I've got it working now thanks to all your insights. — jmg414, Jul 31 '15 at 20:57

score 1 · Answer 1 · answered Jul 31 '15 at 18:11

You're assuming that the plaintext letters are the same case as the key letters. The problem you're seeing with the capital M not being encoded correctly is because of this.

First convert all characters in the key to upper case:

    if (!isalpha(key[i]))
    {
        printf("Invalid command. Please specify key.");
        return 1;
    } else {
        key[i]=toupper(key[i]);
    }

Then when you're encoding, assume the key is uppercase. Also, instead of adding or subtracting the ASCII values for A and a, just use the character constants instead. It makes your code more readable.

You also don't need separate isupper and islower checks since you're already calling isalpha at the prior level up.

        if (isupper(plain[i]))
        {
            plain[i] = (((plain[i] - 'A') + (key[j%key_length] - 'A')) % 26) + 'A';
            j++;
        }
        else 
        {
            plain[i] = (((plain[i] - 'a') + (key[j%key_length] - 'A')) % 26) + 'a';
            j++;
        }

score 0 · Answer 2 · answered Jul 31 '15 at 18:11

There are four cases to consider

plain[i]    key[j]
------------------
 lower      lower
 lower      upper
 upper      lower
 upper      upper

Your code only handles two of those cases.

Side note: 65 should be written as 'A' and similarly for all of the other hard-coded numbers.

CS50 pset2 Vigenere code - Outputs one incorrect letter

2 Answers2