Caesar Cipher that takes an arbitrary offset

This Caesar cipher can take any integer and it will wrap around the correct number of times and still perform the encryption/decryption.

Let's say we have a text file named dog.


~$ cat dog

The quick brown fox jumps over the lazy dog...

If the user enters a normal offset (1-26), it will perform like any other Caesar cipher.


~$ ./caesar dog 1

~$ cat dog

Uif rvjdl cspxo gpy kvnqt pwfs uif mbaz eph...

Negative numbers can be used to shift backwards.


~$ ./caesar dog -1

~$ cat dog

The quick brown fox jumps over the lazy dog...

Now, here's the beauty of it. Choosing an offset of 1 is no different then choosing an offset of, say, 53 or -51. This algorithm will correctly wrap around.


~$ ./caesar dog -51

~$ cat dog

Uif rvjdl cspxo gpy kvnqt pwfs uif mbaz eph...

~$ ./caesar dog 51

~$ cat dog

The quick brown fox jumps over the lazy dog...

I wrote this a long time ago and I posted it on some other programming site back in the day. I haven't really written much pure c code since. I'm guessing there's plenty of room for improvement.

Compiles with:

gcc -ansi -pedantic -Wall -Werror -O3 -o caesar caesar.c

```
#include
#include
#include

/ #define offset (-1) / / My chosen default offset /
#define lstart (0x61) / Start of lowercase ASCII alphabet /
#define ustart (0x41) / Start of uppercase ASCII alphabet /
#define nalpha 26 / Number of letters in my alphabet /

int main(int argc, char **argv) {
int ec, wrap, offset;
FILE *fp;

if (argc != 3) {
fprintf(stderr, "Usage: %s \n", argv[0]);
return 1;
}

fp = fopen(argv [1], "r+b");

offset = atoi (argv[2]);

if(!fp) {
fprintf(stderr, "Error: could not open file!\n");
return 1;
}

while((ec = fgetc(fp)) != EOF) {
if(isalpha(ec)) {
if(islower(ec)) {
wrap = (ec + offset - lstart) % nalpha;
wrap = (wrap < 0) ? nalpha + wrap : wrap;

Some comments on your updated code in Rev 3:

-
I would define your constants as follows:

#define LSTART ('a')
#define USTART ('A')
#define NALPHA ('z' - 'a' + 1)

You now need no comments - they are obvious.

-
It would be normal to use perror(argv[1]); to print the error on opening
the file.

-
My man-page says atoi has been depracated in favour of strtol

-
return values from main are traditionally EXIT_FAILURE and EXIT_SUCCESS

-
I would define offset and wrap at their point of first use. Also it is
generally recommended not to define multiple variables on one line.

-
In the while loop you should put the fputc(ec, stdout); outside the
if(isalpha(ec)) {...} condition. That way the format of what you encrypt
is maintained. Equally, I would omit printing \n at the end so that the
output file (on stdout) has the same form as the input.

-
I prefer to see a space after keywords, if, while etc.

-
Don't overuse ?: - it is not a universal replacement for if-else

-
I find a character named ec to be less intuitive than one named ch, but
maybe that is just me.

-
I think your encryption code:

int ascii_offset = islower (ec) ? LSTART : USTART;

    wrap = (ec + offset - ascii_offset) % NALPHA;
    wrap = (wrap < 0) ? NALPHA + wrap : wrap;
    ec = islower(ec) ? wrap + LSTART : wrap + USTART;

would be better written:

const int ascii_offset = islower(ec) ? LSTART : USTART;
    int wrap = (ec + offset - ascii_offset) % NALPHA;
    if (wrap < 0) {
        wrap += NALPHA;
    }
    ec = wrap + ascii_offset;

StackExchange Code Review Q#37336, answer score: 6