Thread: Caesar Cipher in C

Hi,
I was wondering if anyone could review my code and suggest how to improve or optimize this code. I'm very new to C and programming.

Code:

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


int euclideanMod(int a, int b) {
  int mod = a % b;
  if (mod < 0) mod = (b < 0) ? mod - b : mod + b;
  return mod;
}


int str_indexOf(const char* str, char c)
{
    int index = -1;
    char* pos = strchr(str, c);
    if (pos != NULL) index = pos - str;
    return index;
}


void getChoice(const char* msg, char* c)
{
    do
    {
        printf("%s", msg);
        scanf(" %c", c);
        if (*c == 'd' || *c == 'e') break;
        printf("wrong choice selected!\n");
    } while (1);
}


void getKey(const char* msg, int* k)
{
    do
    {
        printf("Please select secret key to %s: ", msg);
        scanf(" %d", k);
        if (*k >= 1 && *k <= 26) break;
        printf("key length must be 1 to 26\n");
    } while (1);
}


char shiftChar(int key, const char* alp_Arr, char c, _Bool encrypt)
{
    char e_C = c;
    _Bool is_upper = isupper(c);
    if (is_upper) e_C += 32;
    int index = str_indexOf(alp_Arr, e_C);
    if (index != -1)
    {
        if (encrypt)
        {
            e_C = alp_Arr[euclideanMod((index + key),  26)];
        }
        else e_C = alp_Arr[euclideanMod((index - key),  26)];
    }
    if (is_upper) e_C -= 32;
    return e_C;
}


void cipher(const char* msg, char* outMsg, size_t buff_size, int key, const char* alp_Arr, _Bool encOrDec)
{
    char enc_msg[buff_size];
    int msg_len = (int)strlen(msg);
    for (int i = 0; i < msg_len; i++) enc_msg[i] = shiftChar(key, alp_Arr, msg[i], encOrDec);
    strcpy(outMsg, enc_msg);
}

int main()
{
    char msg[300], out[300];
    _Bool enOrDe;
    int k;
    char choice;
    getChoice("Please select \"e\" for encrypt or \"d\" for decrypt: ", &choice);
    if (choice == 'e')
    {
        getKey("encrypt", &k);
        enOrDe = 1;
    }
    else
    {
        getKey("decrypt", &k);
        enOrDe = 0;
    }
    getchar(); // removes trailing '\n' from last scanf() by getKey()
    printf("please enter your message: ");
    fgets(msg, sizeof(msg), stdin);
    char* pos;
    if ((pos=strchr(msg, '\n')) != NULL) *pos = '\0';
    char alp[26];
    for (int i = 0; i < 26; i++) alp[i] = (char)((97+ i));
    cipher(msg, out, 300, k, alp, enOrDe);
    printf("%s\n", out);
    return 0;
}

Code:

void getChoice(const char* msg, char* c)
{
    do
    {
        printf("%s", msg);
        scanf(" %c", c);
        if (*c == 'd' || *c == 'e') break;
        printf("wrong choice selected!\n");
    } while (1);
}

I would add a blank line under the if statement; this is a stylistic choice; I read the next line as being part of the if by mistake; and,
edit the code below to show how I would avoid this dumb mistake that others might make.

Code:

        if (*c == 'd' || *c == 'e') break;

        printf("wrong choice selected!\n");

Also, I would have getChoice return the char instead of passing by address. But, this is a stylistic choice where either is correct.

Tim S.

Originally Posted by stahta01

I would add a blank line under the if statement; this is a stylistic choice; I read the next line as being part of the if by mistake

Instead of doing that, always use braces for these if statements and loops, even if the body of the if statement is only one statement. That is:

Code:

if (*c == 'd' || *c == 'e')
{
    break;
}

This avoids variants of the goto fail bug, which basically is a case of formatting tricking both author and reviewer. Adding a blank line could help visually, but then blank lines are commonly used to separate logical portions of code, whereas it is quite possible that the code following an if statement should be grouped with it.

By the way, note that this is a variable length array:

Code:

char enc_msg[buff_size];

This was made part of the C standard back in 1999, but apparently adoption was poor so they reversed course and now it is merely an optional part of standard C in the 2011 edition of the C standard.