Caesar cipher won't work in C

Question

I am trying to create a Caesar cipher, so I declared a function that pushes every character 13 times. And then another function that takes a string and then encrypts or decrypts it, I did that with the for loop. But the problem is that when I run the programm this is what comes out:

Original: This is the Original text
encrypted: QnÇ vÇü qre Bevtvanyüràü
decrypted: DaÇ âÇü der Orâüânaåüeàü

Does anybody have an idea, what might be causing this?

#include <stdio.h>
#include <stdlib.h>

int shift = 13;

char shiftchar(char ch){
    if( ((ch > 64) && (ch< 91)) || ((ch > 96) && (ch < 123)) ){
        ch = ch + shift;
        if(ch > 90 && ch < 104){
            ch = ch - 90 +64;
        }
        else if(ch > 122 && ch < 136){
            ch = ch -122 +96;
        }
    }
    else{
    ch = ch;
    }
}

void cipher (char str[]){

    for( int i = 0; str[i] != 0; ++i){
        str[i] = shiftchar(str[i]);
    }
}

int main(void){

    char str[25] = "This is the original text";

    printf("Original: ");
    printf("%s\n", str);

    cipher(str);
    printf("encrypted: ");
    printf("%s\n", str);

    cipher(str);
    printf("decrypted: ");
    printf("%s\n", str);
}

Answer 1

You had two simple bugs with your code. First, a variable of type char takes up 8 bits in memory with values ranging form -128 to 127. In some cases, your ciphered character might end up with a value greater than 127 so you need to use an unsigned char as the data type which takes values ranging from 0 to 255 instead.

So, your function declaration should look like : char shiftchar(unsigned char ch);

There is also no need for this line of code : ch = ch; and you also didn't return the resulting ciphered character from your function.

The final function should look like this :

char shiftchar(unsigned char ch){
    if( ((ch > 64) && (ch< 91)) || ((ch > 96) && (ch < 123)) ){
        ch = ch + shift;
        if(ch > 90 && ch < 104){
            ch = ch - 90 +64;
        }
        else if(ch > 122 && ch < 136){
            ch = ch -122 +96;
        }
    }
    return ch;
}

Your code will be functioning as desired by now but to have everything perfectly done we need to modify the size of your character array. You set the size of the array char str[] in main to 25.

Never forget the invisible NULL terminating character at the end of any string literal you use to initialize your array.

The array declaration should be : char str[26] = "This is the original text";, you can also avoid the headache of forgetting the \0, you can do this instead : char str[] = "This is the original text";thus leaving the job to the compiler to determine the size of the array ;)

Answer 2

You seem to have forgotten that a char string is really called null-terminated byte string. The null-terminator is extremely important, since that's what all standard functions are looking for to know when and where the string ends.

You array str is to small to fit the terminator. Make it an array of 26 elements instead. Or don't specify a size explicitly and let the compiler set it.

Answer 3

I tested your code and, besides the unsigned issue, you are missing the return statement in the shiftchar function.

char shiftchar(unsigned char ch){
    if( ((ch > 64) && (ch< 91)) || ((ch > 96) && (ch < 123)) ){
        ch = ch + shift;
        if(ch > 90 && ch < 104){
            ch = ch - 90 +64;
        }
        else if(ch > 122 && ch < 136){
            ch = ch -122 +96;
        }
    }
    else{
    ch = ch;
    }
    return ch;
}