Caesar Cipher program in C

Question

I'm a beginner-intermediate C++ programmer, and I never used or understood C input & validation. I just always used C++ streams.

Anyway, I just want code critique, as I have never used the C input functions (I admit, I have used and like printf()! I even made my own sprintf(), compatible with the C++ string class.)

I hope my code is good. I tend to get code to work properly, completely ignoring formatting and performance, then later once it works I make it look and run nicely.

caesar.c

#include <string.h> /* for strlen() */
#include "caesar.h"

char *getCipherText(char *src, int key) {
    int len = strlen(src);
    int i = 0;
    int ch = 0;

    /* Loop over each char in src */
    for (i = 0; i < len; i++) {
        ch = (int)src[i]; /* Convert the char to int to prevent many uneccecary casts */
        if (ch >= 65 && ch <= 90) { /* If the char is uppercase */
            ch += key; /* add the key */
            if (ch > 90) ch -= 26; /* if the char is higher than the highest uppercase char, sub 26 */
            if (ch < 65) ch += 26; /* if the char is lower than the lowest uppercase char, add 26 */
            src[i] = (char)ch; /* set the current char in src to the char value of ch */
        } else if (ch >= 97 && ch <= 122) { /* else if it's lowercase */
            ch += key; /* add the key */
            if (ch > 122) ch -= 26; /* if the char is higher than the highest lowercase char, sub 26 */
            if (ch < 97) ch += 26; /* if the char is lower than the lowest lowercase char, add 26 */
            src[i] = (char)ch; /* set the current char in src to the char value of ch */
        } 
        /* an else case is not needed, since we are modifying the original. */
    }
    /* Return a pointer to the char array passed to us */
    return src;
}

char *getPlainText(char *src, int key) {
    /* Since getCipherText adds the key to each char, adding a negative key 
     * is equivalent to subtracting a positive key. Easier than re-implementing.
     */
    return getCipherText(src, -key);
}

caesar.h

char *getCipherText(char *src, int key);
char *getPlainText(char *src, int key);

main.c

#include <stdio.h> /* for printf(), sscanf(), fgetc() and fgets() */
#include <stdlib.h> /* for malloc() and free() */
#include <string.h> /* for strlen() */
#include "caesar.h"

/* Size of text buffer to read into */
#define BUFS 1024

/* Size of buffer for reading misc. items, i.e. for the get_int() function */
#define TBUFS 128

/* Get char, no new lines */
int getc_nnl() {
    int ch = '\n';

    /* While ch is a newline or carriage return, read another char */
    while (ch == '\n' || ch == '\r')
        ch = fgetc(stdin);

    /* Return the char, promoted to an integer */
    return ch; 
}

/* Get an integer */
int get_int() {
    char *s = (char*)malloc(TBUFS); s[0] = '\0';
    int i = 0;
    /* Read a string, and later parse the int in case there are no ints to read */
    while (strlen(s) <= 1) 
        fgets(s, TBUFS, stdin);

    /* Parse the int. Using sscanf because scanf on stdin leaves the file caret at a unknown position. */
    sscanf(s, "%d", &i);

    /* Return the int - if sscanf found nothing, it's already 0 */
    return i;
}

int main(int argc, char *argv[]) {
    char *text = NULL;
    int key = 0;
    int ch = 0;

    while(1) {  /* Forever loop, until we break; out */

        /* Prompt for an option */
        printf("Encrypt, Decrypt, or Quit? (e/d/q) ");
        ch = getc_nnl(); 

        /* Make sure they gave us a valid option - if not, keep prompting! */
        while (ch != 'e' && ch != 'E' && ch != 'd' && ch != 'D' && ch != 'q' && ch != 'Q') {
            printf("Invalid option. Encrypt, Decrypt, or Quit? (e/d/q) ");
            ch = getc_nnl();
        }

        /* If the user wants to quit... */
        if (ch == 'q' || ch == 'Q') 
            break; /* ...then break out of the loop */

        /* Allocate buffer for text (I set text[0] to a null-terminator due to later strlen() calls) */
        text = (char*)malloc(BUFS); text[0] = '\0';

        /* Get the text to encrypt - If user entered nothing,
         * or fgets() is reading only a newline (from the fgetc calls), keep reading.
         */
        printf("Please enter your text: ");
        while (strlen(text) <= 1)
            fgets(text, BUFS, stdin);

        /* Get the integer key to encrypt/decrypt with. If it's invalid, keep prompting! :) */
        printf("Ok, now what key should I use? (1 - 25) ");
        key = get_int();
        while (key < 1 || key > 25) {
            printf("Invalid key. Please enter a number between 1 and 25: ");
            key = get_int();
        }

        /* Ok, we have our data - now, did they say encrypt or decrypt? */
        if (ch == 'e' || ch == 'E') {
            /* Encrypt, and print the result */
            getCipherText(text, key);
            printf("Ok. Here's your encrypted text: %s\n", text);
        } else if (ch == 'd' || ch == 'D') {
            /* Decrypt, and print the result */
            getPlainText(text, key);
            printf("Ok. Here's your decrypted text: %s\n", text);
        }

        /* Free our malloc, so we don't have a memory overrun */
        free(text);
    }

    return 0;
}

Answer 1

/* Loop over each char in src */
for (i = 0; i < len; i++) {

To some C programmers, writing string manipulation like this is like speaking in a foreign accent. Consider this:

while (*src)
{
    // TODO: do something with *src

    src++;
}

Moreover, the way you've written it you're looping through all chars twice. (strlen will loop through all chars to find the length, and then you'll loop through again to perform the cypher.) The code above loops through exactly once.

ch = (int)src[i]; /* Convert the char to int to prevent many uneccecary casts */

I'm not sure what is meant by this comment, or where the confusion lies. I'd say do it without casts if you can. (It looks like you can.)

if (ch >= 65 && ch <= 90) { /* If the char is uppercase */

Are you hardcoding ASCII? That's a little weird. You can do ch >= 'A' etc. Better yet, isupper from ctype.h.

while (strlen(s) <= 1) 
    fgets(s, TBUFS, stdin);

This looks very weird. First of all strlen(s) <= 1 should produce a result equivalent to !s[0] || !s[1], except that strlen will traverse the entire string, which is bad. But overall the "while length of string is <= 1" approach is a bit confusing... I think cleaner code would call fgets first, then observe the result.

Update, having glanced at the code again: Also, your buffer sizes are small and fixed-size. At those sizes it's probably more sensible to make them stack-allocated arrays instead of calling malloc.

Answer 2

I think I'd change a few things. First of all, I would generally avoid making the program interactive. At least for me, a program like this that fundamentally carries out a single operation should (by strong preference) let me enter everything necessary on the command line. If you insist, you can allow interactive use as well, but don't force me to respond to your prompts instead of entering data as I see fit, and don't stop me from creating scripts and such to handle such things with even less work if I choose to.

Second, I think I'd segment the problem into functions slightly differently. At the bottom level, I'd have a function that just takes one input character, and returns one encoded/decoded character:

int cipher(int ch, int key) {   
    if (islower(ch)) return (ch-'a' + key) % 26 + 'a';
    if (isupper(ch)) return (ch-'A' + key) % 26 + 'A';
    return ch;
}

Then, if you really want to deal in strings, as a layer on top of that, I'd have something that knows how to deal with strings of characters:

void encode(char *s, int key) { 
    while (*s)
        *s = cipher(*s++, key);
}

void decode(char *s, int key) { 
    while (*s) 
        *s = cipher(*s++, -key);
}

Then, as the top layer enough to open files, and act as a filter:

int main(int argc, char **argv) { 
    char buffer[256];
    // skipping error checking for now.
    int key = atoi(argv[1]);

    while (fgets(buffer, 256, stdin))
        if (tolower(argv[2][0] == 'd')
            puts(decode(buffer));
        else
            puts(encode(buffer));
    return 0;
}

Or, if you don't mind a tiny bit of trickiness:

typedef int (*func)(int);

func f = tolower(argv[2][0]) == 'd' ?
       decode : 
       encode;

while (fgets(buffer, 256, stdin))
    puts(f(buffer));

Alternatively, you can ignore using strings at all, and just deal in single characters throughout:

int encode(int ch, int key) { 
    if (islower(ch)) ch =  (ch-'a' + key) % 26 + 'a';
    else if (isupper(ch)) ch = (ch-'A' + key) % 26 + 'A';
    return ch;
}

int decode(int ch, int key) { 
    return encode(ch, -key);
}

int main(int argc, char **argv) { 
    int ch;
    int key = atoi(argv[1]);

    int (*f)(int) = argv[2][0] == 'd' ?
        decode :
        encode;

    while (EOF != (ch=getchar()))
         putchar(f(ch));

    return 0;
}

Answer 3

• after malloc check result to NULL
• getCipherText check src to NULL, if strlen get NULL string it crushed.
• function get_int free s string before return result