-2

I am a beginner trying to learn to code. Currently I am doing the CS50 course. I have encountered a problem with the Vigenere cipher problem; please see my code on a github link below.

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

#define ASCII_VALUE_LOWER 97
#define ASCII_VALUE_UPPER 65
#define NR_OF_LETTERS 26

int main(int argc, string argv[])
{
    char key[strlen(argv[1]) + 1];
    strcpy(key, argv[1]);
    int keyLen = strlen(key);

    for (int k = 0; k < keyLen; k++)
    {
        if (!isalpha(key[k]))
        {
            printf("ERROR: Secret key has to be alphabetical string, program will be terminated now!\n");
            return 1; // main should return 1 (signify an error)
        }
        //converting key letters to respective values
        if (isupper(key[k]))
            {
                key[k] -= ASCII_VALUE_UPPER;
            }
            key[k] -= ASCII_VALUE_LOWER;
    }
    //if program is executed without an argument, or with more than one arguments
    if (argc != 2)
    {
        printf("ERROR: You need to give a secret key as an argument, program will be terminated now!\n");
        return 1; // main should return 1 (signify an error)
    }
    else
    {
        string plaintext = get_string("plaintext: "); //get a plaintext from a user using cs50 custom function from their library
        int stringLen = strlen(plaintext);
        int keyIndex = 0;

        for (int j = 0; j < keyLen; j++)
        {

        }

        //for each character in the plaintext string
        for (int i = 0; i < stringLen; i++)
        {
            //check if is alphabetic (tolower, toupper)
            if (isalpha(plaintext[i]))
            {
                //cypher_character = (plain_character + key_character)% 26
                if (islower(plaintext[i]))
                {
                    keyIndex %= keyLen;
                    plaintext[i] = ((plaintext[i] - ASCII_VALUE_LOWER + key[keyIndex]) % NR_OF_LETTERS) + ASCII_VALUE_LOWER;
                }
                else
                {
                    plaintext[i] = ((plaintext[i] - ASCII_VALUE_UPPER + key[keyIndex]) % NR_OF_LETTERS) + ASCII_VALUE_UPPER;
                }
                keyIndex++;
            }
            //else leave as is
        }

        //print ciphertext in a format "ciphertext: " + ciper
        printf("ciphertext: %s\n", plaintext);
        return 0;
    }
}

The issues are as following:

  1. if you pass an argument in uppercase letters in a key, the values are weird and the conversion doesn't work. The idea is to take each character in a string key and subtracting 65 if uppercase, or 97 if lowercase, so their ASCII value would go to 0 - 25. I then can make a use of them in a Vigenere cipher formula: cipher[i_index] = (plaintext[i_index] + key[j_index]) % 26

  2. doesn't handle lack of argv[1], even though there is an IF condition (!argc == 2), so it shouldn't go through if you don't pass anything.

    "failed to execute program due to segmentation fault".

I have tried everything in my capability, I am pretty tired, maybe tomorrow the solution will popout instantly. I ask you to give me some hints, possibly not revealing everything, but maybe guiding me, so I can learn from it.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Adam Straka
  • 9
  • 2
  • Why don't you just change everything to uppercase and then subtract 65? – Robert Harvey Aug 25 '18 at 18:02
  • Have you tried debugging your program? – Lasse V. Karlsen Aug 25 '18 at 18:02
  • ...and then subtract `'A'` is what @RobertHarvey means. – Paul Ogilvie Aug 25 '18 at 18:03
  • Your second issue occurs because by the time you check whether `argc != 2`, you have already accessed `argv[1]`, and thereby assumed that `argc >= 2`. If the caller hasn't supplied an argument, then `argv[1]` is past the end of the array and evaluating that expression results in undefined behavior. – Joe Farrell Aug 25 '18 at 18:13
  • @Joe Farrell, thank you for the heads up, this fixed the issue! – Adam Straka Aug 25 '18 at 18:31
  • Take a close look at this, and consider how keyIndex will update if the string stored in plaintext is composed entirely of upper case letters. You've already identified this isn't working, if you look closely it's obvious why. – dgnuff Aug 25 '18 at 19:04

2 Answers2

3
    if (isupper(key[k]))
        {
            key[k] -= ASCII_VALUE_UPPER;
        }
        key[k] -= ASCII_VALUE_LOWER;

If the character is upper case, this subtracts ASCII_VALUE_UPPER. And then, no matter what, it subtracts ASCII_VALUE_LOWER. From the surrounding code, I assume you meant:

    if (isupper(key[k])) {
        key[k] -= ASCII_VALUE_UPPER;
    } else {
        key[k] -= ASCII_VALUE_LOWER;
    }
Rob Napier
  • 286,113
  • 34
  • 456
  • 610
  • Thank you! I've been staring at it for past hours and tried to figure out where is the problem... sadly I am not very efficient with debugger, so I couldnt identify where the problem originates. This makes the program function better, but still there is some bug burried. for example giving a key BaZ on a word BaRFoo gives CaQFon, but it should be CaQGon. if I put FFFfff for the same key (BaZ) it works as it should, giving GFEgfe. – Adam Straka Aug 25 '18 at 18:21
  • Rather than struggle with the debugger (an important skill, but let's put that to the side for the moment), use `printf()`. At each step along the way, print out what the current values are and make sure they're what you want. – Rob Napier Aug 25 '18 at 18:25
  • 1
    ok, so I did as you suggested and finally I found the culprit. it was poorly placed variable operation inside the if condition, instead of outside of it. thank you again for your help! – Adam Straka Aug 25 '18 at 18:53
1

As others suggested, everything was fixed, if someone would be curious where exacly was the mistake:

1.] Mistake pinpointed by @Rob Napier

for (int i = 0; i < stringLen; i++)
    {
        //check if is alphabetic (tolower, toupper)
        if (isalpha(plaintext[i]))
        {
            keyIndex %= keyLen; // makes sure that keyIndex doesnt exceeds actual string length
            //cypher_character = (plain_character + key_character)% 26
            if (islower(plaintext[i]))
            {
                plaintext[i] = ((plaintext[i] - ASCII_VALUE_LOWER + key[keyIndex]) % NR_OF_LETTERS) + ASCII_VALUE_LOWER;
            }
            else
            {
                plaintext[i] = ((plaintext[i] - ASCII_VALUE_UPPER + key[keyIndex]) % NR_OF_LETTERS) + ASCII_VALUE_UPPER;
            }
            keyIndex++;
        }
        //else leave as is
    }

keyIndex %= keyLen; was placed inside of the if condition below it, so it didnt execute in every iteration of the FOR loop.

2.] answered by @Joe Farrel: because by the time I checked whether argc != 2, I have already accessed argv[1], and thereby assumed that argc >= 2. If the caller hasn't supplied an argument, then argv[1] is past the end of the array and evaluating that expression results in undefined behavior. - So I moved the if condition as a first thing in main.

Adam Straka
  • 9
  • 2