-1

My goal is to make a Vigenere cipher. I am attempting to do this by getting the key from argv, getting the string from the user, then passing the message and key though a function I made which combined them and returns the new value before printing it. For some reason it is just printing the key. I think it has something to do with the new function and how I am trying to use the returned value. Here is the code:

#include <stdio.h>
#include <stdlib.h>
#include <cs50.h>
#include <string.h>
#include <stdbool.h>
#include <ctype.h>
int new_crypt;
int encrypt(string , int );
int main(int argc, string argv[])
{
    if( argc != 2)
    {
        printf("Plese enter only one key");
        return 1;
    }

    string message = GetString();
    for(int i=0; i < strlen(message); i++)
    {
        int klen = strlen(argv[1]);
        //repeats the key until the message is over
        int key= argv[1][i%klen];
        bool kupper = isupper(key);
        bool klower = islower(key);
        bool kalpha = isalpha(key);
        if(kupper == true){
            //ASCII value of A is 65. 'A' = 0 shifts
            int k = key-65;
            int new_crypt = encrypt(message, k);
            printf("%c", new_crypt);
        }
        if(klower == true){
            //ASCII value of 'a' is 97. 'a' = 0 shifts
            int k = key- 97;
            int new_crypt = encrypt(message, k);
            printf("%c", new_crypt);
        }
        if(kalpha == false){
            int k = 0;
            int i = i-1;
            int new_crypt = encrypt(message, k);
            printf("%c", new_crypt);
        }

    }

    printf("\n");
    return 0;
}
int encrypt(string message, int k)
{
    for(int i=0; i < strlen(message); i++)
    {
        bool upper = isupper(message[i]);
        if(upper == true)
        {   //Makes sure the message doesnt go past 'Z'.. If it does it mod 90 it                                            /                   // and adds 65 ('A')
            int crypt = (message[i]+ k) % 90;
            if(crypt < 65)
            {
                int new_crypt = (crypt + 65);
                return new_crypt;
            }
            else{
                int new_crypt = crypt;
                return new_crypt;
            }
        }
        bool lower = islower(message[i]);
        if(lower == true)
        {
            int crypt = (message[i]+ k) % 123;
            if(crypt < 97)
            {
                int new_crypt = crypt + 97;
                return new_crypt;
            }
            else{
                int new_crypt = crypt;
                return new_crypt;
            }
        }
        bool alpha = isalpha(message[i]);
        if(alpha == false)
        {
            int new_crypt = message[i];
            return new_crypt;
        }
    }
    return 0;
}
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Corvertbibby
  • 66
  • 3
  • 9
  • 7
    What the heck is `string`? – melpomene Nov 30 '12 at 07:14
  • I'm still digesting the sanity of the two if-else-blocks in the for-loop of `encrypt()`.I'll get to `string` in a moment... – WhozCraig Nov 30 '12 at 07:24
  • 2
    Post the real code, with header includes. – Lundin Nov 30 '12 at 07:29
  • I am using some premade headers provided by the class I am taking... My question is purely on using the return value of `encrypt()` in the `main` function. The two if-else statements check if the message char is a capital, lower, or non-char. if it is, then it makes `crypt` by adding the message char i with the key. If the char goes past, for example 90 (ascii for Z) + 1, then it mods the number and gives 1. Since 1 is not equal to A, I have to check if it is < 65(ascii for A) and if so add 64 and give the result A. I'm sure theres a better way of doing this, but this is what I got. – Corvertbibby Nov 30 '12 at 09:08
  • Also, beware of `int new_crypt` with `"%c"`. – Eregrith Nov 30 '12 at 15:15

1 Answers1

1

The loop in the encrypt function is completely useless, because there is no path through the loop-body without a return statement causing the loop to be terminated and control returned to the caller of encrypt. This makes that the program as a whole repeatedly encrypts the first character of the message with successive elements from the key.

The easiest way around this is to make the following changes

  • Remove the loop from the encrypt function
  • Pass, as an additional argument, the element from the message that you want to encrypt, making the signature

    int encrypt(string message, int k, int i)
    

Some miscellaneous remarks:

  • The global variable new_crypt is not used anywhere. You can safely remove it. (You should avoid the use of global variables as much as reasonably possible).
  • Instead of using the magic number 65, you can also use the character literal 'A'. This has the advantage that you don't need a comment to explain the number 65 and that it is always the correct value for the capital A, even if you end up not using ASCII. (But see also the next bullet)
  • Your code assumes the letters A to Z (and a to z) have contiguous values (such that Z == A+26). This may happen to be the case for the English alphabet in the ASCII encoding, but it is not guaranteed for other language alphabets or encodings.
Bart van Ingen Schenau
  • 15,488
  • 4
  • 32
  • 41
  • Thank you! I didn't even know I could pass `int i` like that to the other function but now that you mention it, it makes perfect since. Sorry for my ignorance, only my second week of this. And thanks for the remarks, I'll make the changes now. – Corvertbibby Nov 30 '12 at 09:26