Caesar Cipher program in C

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  /* 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 = 65 && ch  90) ch -= 26; /* if the char is higher than the highest uppercase char, sub 26 */
            if (ch = 97 && 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 / for printf(), sscanf(), fgetc() and fgets() /
#include / for malloc() and free() /
#include / 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() {
i

/* 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.

StackExchange Code Review Q#2314, answer score: 7