1

I wrote a code for caesar's cipher and the code works except I can't cipher more than 8 letters and I also can't handle spaces. It shows ">>" this symbol instead of spaces. Also, I wanted to do binary search in the second function of my code but I don't know whether I have done that or not.

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

char caesar (char x, char alphabets[]);

int j;

int main()

{

    char* plain_text = malloc (10 * sizeof(char)  + 1);
    int key, num;
    char* cipher_text = malloc (10 * sizeof(char)  + 1);

    printf("Plain text: ");
    gets(plain_text);
    printf("\nThe plain text is:  ");
    puts(plain_text);
    printf("\nKey: ");
    scanf("%d", &key);
    num = (int)key;

    if (key != num)
    {
        return 1;
    }
    int i;
    char alphabets[] = {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z'};
    for (i=0;i<=strlen(plain_text);i++)
    {
        char x = plain_text[i];
        caesar(x, alphabets);
        cipher_text[i] = alphabets[j+key];
        printf("%c", cipher_text[i]);
    }
    free(plain_text);
}

char caesar (char x, char alphabets[])

{

    if(x == alphabets[13])
    {
        return 13;
    }
    for(j = 1; j <= 13; j++)
    {
        if(x == alphabets[j])
        {
            return j;
        }
    }
    for(j = 13; j <= strlen (alphabets); j++)
    {
        if(x == alphabets[j])
        {
            return j;
        }
    }
}
  • 2
    Never ***ever*** use the `gets` function. It's so [dangerous](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used) that is has been removed from C altogether. Use e.g. [`fgets`](https://en.cppreference.com/w/c/io/fgets) instead. – Some programmer dude Aug 26 '19 at 10:20
  • 1
    And why the cast in the assignment `num = (int)key`? Both `num` and `key` are both of type `int`. And after the assignment then `key != num` will *always* be false. – Some programmer dude Aug 26 '19 at 10:22
  • And are you really trying to cipher the null terminator as well? That doesn't sound correct. – Some programmer dude Aug 26 '19 at 10:23
  • Finally, what are you doing with the value that the function `caesar` returns? And what happens if the last loop ends without you returning a value? – Some programmer dude Aug 26 '19 at 10:23
  • Oh and a *last* final note: Please [learn how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). Even some simple [rubber duck debugging](https://en.wikipedia.org/wiki/Rubber_duck_debugging) would have told you about the `caesar` function problem. And stepping through your code line by line in a debugger, while monitoring values of variables and checking results of all calculations, should have helped you figure out what's wrong. – Some programmer dude Aug 26 '19 at 10:31
  • `sizeof(char)` is _always_ 1 by definition so its use in the `malloc()` calls serves no purpose. – Clifford Aug 26 '19 at 11:21
  • 2
    `strlen (alphabets)` is non-deterministic since`alphabets` is not a nul-terminated string. `const char* alphabets = "abcdefghijklmnopqrstuvwxyz" ;` would make more sense. – Clifford Aug 26 '19 at 11:26
  • `caesar()` tests `alphabets[13]` in three places, but will return immediately after the first. The test `j <= 13` may be simplified to `j < 13`, and the initialisation`j = 13` changed to `j = 14` to avoid both redundant tests. All looks flaky in any case - `alphabets[0]` is never even referenced. – Clifford Aug 26 '19 at 11:33

1 Answers1

2

caesar() appears to simply return the position of characters b to z in the array in a remarkably over complex manner and omits a altogether! Moreover since alphabets is not a null terminated string strlen() is not a valid operation in any case. The "encryption" is done (incorrectly) by alphabets[j+key] in main() making caesar() particularly poorly named - since that is not what it does at all.

The following function will return the the cipher of any character in alphabet, and leave any other character unmodified:

char caesar( char x, int key )
{
    const char alphabet[] = {'a','b','c','d','e','f','g','h',
                             'i','j','k','l','m','n','o','p',
                             'q','r','s','t','u','v','w','x',
                             'y','z'};

    char cipher = x ;

    for( int i = 0; 
         cipher == x && i < sizeof( alphabet ); 
         i++ )
    {
        if( alphabet[i] == x )
        {
            cipher = alphabet[(i + key) % sizeof( alphabet )] ;
        }
    }

    return cipher ;
}

It makes far more sense to pass key to ceasar() that to pass the constant alphabet, and to do the encryption where is alphabet is "known". Splitting the cipher steps between caesar() and main() as you had done is a poor design and lacks cohesion and has unnecessary coupling.

If the character x appears in alphabet it is modified by alphabet[(i + key) % sizeof( alphabet )] ;. This adds the key as you had, but also "wraps-around" (the % modulo operation), so that for example for key = 1, z wraps around to a rather then referencing a byte beyond the end of the alphabet array as your code had. Critically, if it does not appear in alphabet, it is unmodified - that is why cipher is initialised with x. The loop exits when cipher is modified (cipher != x), or when the end of alphabet is reached.

Then in the iteration of plain_text:

for (i = 0; i <= strlen(plain_text); i++ )
{
    cipher_text[i] = caesar( plain_text[i], key ) ;
}

The <= strlen() here is unusual, but here it ensures the nul terminator is copied to cipher_text - it will not be modified by caesar().

Note that the the above solution only encrypts lower case text (as does your original code). There are other issues and ill-advised practices in your code, discussed in the comments but not perhaps directly relevant to your question, but using the above function, the following complete implementation addresses most of the issues:

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

char caesar( char x, int key ) ;

#define MAX_TEXT 128

int main()
{
    char plain_text[MAX_TEXT] = "" ;
    char cipher_text[MAX_TEXT] = "" ;

    printf( "Plain text: " );
    fgets( plain_text, MAX_TEXT, stdin ) ;

    printf( "\nThe plain text is: %s\n", plain_text ) ;

    printf( "Key: " ) ;
    int key = 0 ;
    scanf( "%d", &key );

    for( size_t i = 0; i <= strlen( plain_text ); i++ )
    {
        cipher_text[i] = caesar( plain_text[i], key ) ;
    }

    printf( "\nThe cipher text is: %s\n", cipher_text ) ;
    return 0 ;
}

Example:

Plain text: abc, xyz

The plain text is: abc, xyz

Key: 1

The cipher text is: bcd, yza

Modification to allow capital letters:

#include <ctype.h>

char caesar( char x, int key )
{
    const char alphabet[] = {'a','b','c','d','e','f','g','h',
                             'i','j','k','l','m','n','o','p',
                             'q','r','s','t','u','v','w','x',
                             'y','z'};

    char cipher = x  ;

    for( int i = 0;
         cipher == x && i < sizeof( alphabet );
         i++ )
    {
        if( alphabet[i] == tolower( x ) )
        {
            cipher = alphabet[(i + key) % sizeof( alphabet )] ;
            if( isupper( x ) )
            {
                cipher = toupper( cipher ) ;
            }
        }
    }

    return cipher ;
}

Here the test alphabet[i] == tolower( x ) ignores the case, and then when a match is fount the application of cipher = toupper( cipher ) if x is upper case, yields the upper case cipher.

Example output:

Plain text: aBc, XyZ 123

The plain text is: aBc, XyZ 123

Key: 1

The cipher text is: bCd, YzA 123

Note that instead of testing cipher = x in the for loop, you could break after cipher is assigned in the loop - reducing the number of tests - but that arguably breaks structured programming "rules" - I would not criticise its use by others, but it is not my preference. You might also use isalpha(x) in this case to skip the loop altogether, but that has implementation defined behaviours for accented characters for example, so if you were to extend the supported "alphabet", it might fail to work as intended.

If you only ever use the characters a to z in the alphabet as you have, then a further simplification is possible where the cipher can be determined arithmetically using character code values:

char caesar( char x, int key )
{
    char cipher = tolower( x ) ;

    if( isalpha( x ) )
    {
        cipher = ((cipher - 'a') + key) % ('z' - 'a' + 1) + 'a' ;
        if( isupper( x ) )
        {
            cipher = toupper( cipher ) ;
        }
    }

    return cipher ;
}

Strictly this assumes that the characters a to z are contiguous in the target character set, but that is universally true for any system you are likely to run this code on (i.e. not an IBM Z series mainframe or various antique mainframe/mini computers), and if not the alphabet array solution remains valid. I point this out only because otherwise someone will place a comment about it as if it is really an issue.

To explain the expression: cipher = ((cipher - 'a') + key) % ('z' - 'a' + 1) + 'a' :

  • (cipher - 'a') - subtract the code for 'a' to get a value 0 to 25 for characters a to z.
  • ... + key - add the key "shift"
  • ... % ('z' - 'a' + 1) - this constant expression resolves to % 26 in practice, to "wrap-around".
  • ... +a- transform the range 0 to 25 back into character codesatoz`.
Clifford
  • 88,407
  • 13
  • 85
  • 165
  • Sir, Can you please explain what the function (char caesar( char x, int key ) does? I'm a newbie and it's hard to understand. –  Aug 26 '19 at 12:55
  • 1
    @riki : I have since simplified it and added an explanation - hope that helps. – Clifford Aug 26 '19 at 13:01
  • 1
    @riki : Further simplified (for-loop rather the while used) and solution supporting upper-case added. – Clifford Aug 26 '19 at 16:01
  • 1
    @riki : Arithmetic solution added (and explained). – Clifford Aug 26 '19 at 16:47
  • https://stackoverflow.com/q/57680613/11004300. Sir, Could you see this question please? –  Aug 28 '19 at 06:24