Vigenere Cipher logic error

Question

#include<stdio.h>
#include<cs50.h>
#include<ctype.h>
#include<string.h>
int main(int argc, string argv[]){
int k,j,i=0,ch,pos;
bool apha=true;
string in = GetString();
int num = strlen(in);
    for(int z=0;z<strlen(argv[1]);z++){
        if(!isalpha(argv[1][z])){
        apha=false;
        }
    }
    if(argc!=2||!apha){
    printf("Dude we only accept alphabets...");
    return 1;
    } 
string key = argv[1];
int keylength = strlen(key);
   for (i=0,j=0;i<num;i++,j++){
        if(isupper(key[i])){
            k=key[j%keylength]-'A';
        }
        if(islower(key[i])){
        k=key[j%keylength]-'a';
        }
            if(isupper(in[i])){
                pos=in[i]-'A';
                ch = ((pos + k)%26) + 'A';
                printf("%c",ch);
            }
            if(islower(in[i])){
                pos=in[i]-'a';
                ch = ((pos + k)%26) + 'a';
                printf("%c",ch);
            }
            if(isspace(in[i])){
                printf(" ");
            }
            if(ispunct(in[i])){
                printf("%c",in[i]);
            }
    }
printf("\n");
}

Output condition checks: :) vigenere.c exists

:) vigenere.c compiles

:) encrypts "a" as "a" using "a" as keyword

:( encrypts "world, say hello!" as "xoqmd, rby gflkp!" using "baz" as keyword

\ expected output, but not "xoqkj, yfd gfllp!\n"

:( encrypts "BaRFoo" as "CaQGon" using "BaZ" as keyword

\ expected output, but not "CaQEun\n"

:( encrypts "BARFOO" as "CAQGON" using "BAZ" as keyword

\ expected output, but not "CAQEON\n"

:( handles lack of argv[1]

\ expected output, not a prompt for input

:( handles argc > 2

\ expected output, not a prompt for input

:( rejects "Hax0r2" as keyword

\ expected output, not a prompt for input

What is wrong with my code? I have scrutinized the logic and the error seems to be in the way the key has been wrapped, though I could not find any errors. Where have I gone wrong?

".. \ expected output, not a prompt for input .." did that get a frowny face because there is no code that asks for input? — Jongware, Sep 18 '16 at 12:18
Well maybe... but that it is an easy error (more or less) it's the logical error which is scaring me. — Mayank Goel, Sep 18 '16 at 12:24
SO is mainly useful for asking explicit questions, debugging / fixing code is almost never useful for others. Please ask for help at your institution. — Maarten Bodewes, Sep 18 '16 at 12:54
You are using the wrong index when checking the key: `isupper(key[i])`, also it could be better to "preprocess" the key (pre-calculate the k values) before using it. — Bob__, Sep 18 '16 at 12:58
I know I am not supposed to talk and stuff in SO... but Thanks a lot!!! If anyone's seeing this kind of error, the problem is in the incrementation of the key incrementer (here j). — Mayank Goel, Sep 19 '16 at 12:03

score 1 · Accepted Answer · answered Sep 18 '16 at 17:16

There are several problems with your code:

You're error checking isn't correct. You check if(argc!=2||!apha) after you've already evaluated strlen(argv[1]) -- by then it's too late! Check the validity of argc before accessing argv and don't double up the argument count error and alphabetic key error, they're independent. Also, error messages should go to stderr, not stdout.

You're completely mishandling the key indexing. As @Bob__ noted, the indexing in this code:

if(isupper(key[i])){
    k=key[j%keylength]-'A';
}

needs to be consistent

if (isupper(key[j % keylength])) {
    k = key[j % keylength] - 'A';
}

But also, you're not incrementing j correctly, you have it tracking i:

for (i=0,j=0;i<num;i++,j++){

Instead, i should increment for every character in the input string, j should increment for every encryptable letter in the input string.

Reworking your code to fix the above errors and general style issues, we get something like:

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

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

    if (argc != 2) {
        fprintf(stderr, "Please supply an encryption key.\n");
        return 1;
    }

    string key = argv[1];
    int key_length = strlen(key);
    bool is_alpha = true;

    for (int z = 0; z < key_length; z++) {
        if (!isalpha(key[z])) {
            is_alpha = false;
        }
    }

    if (!is_alpha) {
        fprintf(stderr, "Sorry, we only accept alphabetic keys.\n");
        return 1;
    }

    string in = GetString();
    size_t length = strlen(in);

    for (int i = 0, j = 0; i < length; i++) {

        if (isalpha(in[i])) {

            int ch, k = key[j++ % key_length];

            if (isupper(k)) {
                k -= 'A';
            } else {
                k -= 'a';
            }

            if (isupper(in[i])) {
                int pos = in[i] - 'A';
                ch = ((pos + k) % 26) + 'A';
            } else {
                int pos = in[i] - 'a';
                ch = ((pos + k) % 26) + 'a';
            }

            printf("%c", ch);
        } else if (isspace(in[i])) {
            printf(" ");
        } else if (ispunct(in[i])) {
            printf("%c", in[i]);
        }
    }

    printf("\n");

    return 0;
}

USAGE SIMULATION

> ./a.out baz
world, say hello!
xoqmd, rby gflkp!
>

score 0 · Answer 2 · answered Sep 19 '16 at 12:12

Here is the answer as I got it-

-The most striking error is probably the:

   if(isupper(key[i])){
   k=key[j%keylength]-'A';
   }

It should check for the corresponding character so should check for:

   if (isupper(key[j % keylength])) {
   k = key[j % keylength] - 'A';
   }

-Also, the increment of the key-increment is important, to do that just increment only if it is an alphabet. So an isalpha check for that is needed (as you don't want the character to change even for a space).

Vigenere Cipher logic error

2 Answers2