Caesar cipher in C

Question

I've written a simple program that encrypt and decrypt a string using Caesar cipher.
There are some examples to show how it works:

$ cae encrypt "Hello World!" 5
Mjqqt Btwqi!
$ cae decrypt "Mjqqt Btwqi!" 5
Hello World!

$ cae encrypt "Hello world!"
Ebiil Tloia!
$ cae decrypt "Ebiil Tloia!"
Hello World!

If no key is given, it will encrypt or decrypt the string with three shifts.

The code is organized in a single main.c file and a Makefile.

main.c

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

#define LEFT_SHIFT 1
#define RIGHT_SHIFT 0

#define DEFAULT_KEY 3

/* It shifts a character by 'key' positions to right.
 *
 * Arguments:
 *      chr - the character (is must be between A-Z or a-z);
 *      key - the number of shifts (max 25).
 *
 * Return value:
 *      The shifted character.
 *
 * Note:
 *      If chr is not between A-Z or a-z or key is greater than 25, there is
 *      undefined behaviour.
 */
char __shift_chr(char chr, const unsigned int key)
{
        const int letters = 26;
        int diff = isupper(chr) ? 'A' : 'a';

        return (chr - diff + key) % letters + diff;
}

/* It takes a string and shifts every character by 'key' positions to left or
 * right.
 *
 * Arguments:
 *      chiper - the string to shift;
 *      key - the number of shifts for every character (between 1 and 25).
 *      mode - LEFT_SHIFT (to left) or RIGHT_SHIFT (to right).
 *
 * Return value:
 *      It returns zero on success, otherwise one on error if the key is not
 *      between 1 and 25 or chiper is a NULL pointer.
 */
int __shift_str(char *chiper, unsigned int key, int mode)
{
        const short int letters = 26;

        if (!chiper || (key < 1 && key > 25))
                return 1;

        /* Because chiper function works only with right shift. */
        if (mode == LEFT_SHIFT)
                key = letters - key;

        while (*chiper != '\0') {
                if (isalpha(*chiper))
                        *chiper = __shift_chr(*chiper, key);

                chiper++;
        }

        return 0;
}

/* It encrypt a null-terminated byte string with `key` left shifts.
 *
 * Arguments:
 *      - *str - the null-terminated byte string;
 *      - key - number of shifts (between 1 and 25).
 *
 * Return value:
 *      Zero on success, otherwhise a positive value.
 */
int cae_encrypt(char *str, const short int key)
{
        return __shift_str(str, key, LEFT_SHIFT);
}

/* It decrypt a null-terminated byte string.
 *
 * Arguments:
 *      - *str - the null-terminated byte string;
 *      - key - number of shifts (between 1 and 25).
 *
 * Return value:
 *      Zero on success, otherwhise a positive value.
 */
int cae_decrypt(char *str, const short int key)
{
        return __shift_str(str, key, RIGHT_SHIFT);
}

int main(int argc, char *argv[])
{
        if (argc != 4 && argc != 3) {
                printf("usage: cae <encrypt|decrypt> <message> [key]\n\n");
                return 1;
        }

        unsigned int key;
        if (argc == 3)
                key = DEFAULT_KEY;
        else
                key = strtoul(argv[3], NULL, 0);


        if (key < 1 || key > 25) {
                fprintf(stderr, "key must be between 1 and 25\n\n");
                return 1;
        }

        char *str = argv[2];
        char *command = argv[1];

        if (!strcmp(command, "encrypt")) {
                cae_encrypt(str, key);
        } else if (!strcmp(command, "decrypt")) {
                cae_decrypt(str, key);
        } else {
                fprintf(stderr, "invalid command\n\n");
                return 1;
        }

        /* We don't check the return value because we checked the arguments
         * before we passed them to the function, so it will always return 0.
         */

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

        return 0;
}

Makefile

.PHONY = clean all tests

SHELL = /bin/sh

CC ?= gcc

CFLAGS = -Wall -Wextra -g

EXEC = cae

OBJECTS = main.o

all : $(EXEC)

$(EXEC) : $(OBJECTS)
    $(CC) -o $@ $^

clean :
    $(RM) *.o $(EXEC)

I think that is not necessary to move functions __shift_chr and __shift_str into a new file, because they are small.
Also I forced the program to encrypt with left shifts (and decrypt with right shifts), but It could be possibile to do the same opposite.

Answer 1

The big assumption

This program works as advertised only if the host character coding has contiguous a..z and A..Z represented as single bytes. That works for the majority of codings used today, including ASCII, ISO 8859.x, and even UTF-8. It will fail on EBCDIC systems, though. You could probably write a small test for the problem platforms (e.g. assert('z'-'a' == 25); assert('Z'-'A' == 25);).

Includes

The program includes <getopt.h> but never uses it. Consider moving all except <ctype.h> down to immediately before main() if you might want to use the functions with a different main() (e.g. GUI or Curses interface).

Naming

Identifiers beginning with underscores, and those containing double underscores, are reserved to the implementation for any purpose. That means they can even be macros! So __shift_str and __shift_chr need to be changed. Given that __shift_chr is used only once, you might consider inlining it (that would obviate the following item).

letters constant

Instead of declaring letters separately in __shift_str and __shift_chr, it would be better to declare it only once - perhaps as a global.

Bug - error checking

This error checking is flawed, because key cannot both be less than 1 and greater than 25:

if (!chiper || (key < 1 && key > 25))

The correction is

if (!chiper || key < 1 || key > 25)

Or (better)

if (!chiper || key <= 0 || key >= letters)

This will have been masked in your testing by the (correct) version in main().

isalpha() matches all letters

In the C locale, you probably get away with isalpha(). In general, we can't assume that it matches only un-accented a..z and A..Z.

Prefer plain int

Use short int only for optimized storage of values. For calculation and argument passing, use int - it may be faster and result in smaller code; more importantly, it reduces the surprises caused by promotions you didn't expect.

No need for separate mode argument.

Instead of modifying key within shift_str, it can be done within decrypt():

int cae_decrypt(char *str, const int key)
{
        const int letters = 26;
        return cae_encrypt(str, letters-key);
}

Check the result of strtoul()

You should check that result of the conversion is not zero. It's probably a good idea to pass a str_end parameter, and check that it points to end of string (and not equal to the input argv[3]) to require exactly a number, and nothing else. You might want to check it's in range for int before assigning it.

Use EXIT_SUCCESS and EXIT_FAILURE

Since we include <stdlib.h>, we may as well take advantage of these macros to better express the return from main().

Prefer puts(str) to printf("%s\n", str)

Since we just want to print a string value and newline, let's use the simpler function; printf() is overkill here.

Usage message should go to stderr

If the wrong number of arguments are passed, we get a message on standard output. This should be on standard error, like the other messages. This avoids situations like the traffic sign that had "Sorry I'm not in the office" instead of the true translation of "This road will be closed"!

The Makefile

The Makefile is very good. Just two things I'd change:

• It's not necessary to write SHELL=/bin/sh - that's the default (and it's not affected by an inherited environment variable).
• You can use $(LINK.c) instead of $(CC) to produce the binary.

I approve of -Wall -Wextra, and suggest also -Wwrite-strings -Warray-bounds. You might get some benefit from -Wconversion, too (e.g. the assignment from strtoul() into an unsigned int).