Caesar Cipher that takes an arbitrary offset

Question

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 <stdio.h>
#include <stdlib.h>
#include <ctype.h>

/* #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 <filename> <offset>\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; 

            } else {
                wrap = (ec + offset - ustart) % nalpha;
                wrap = (wrap < 0) ? nalpha + wrap : wrap; 
            }

            ec = islower(ec) ? wrap + lstart : wrap + ustart;

            fseek(fp, -1L, SEEK_CUR);
            fputc(ec, fp);
        }
    }

    fclose(fp);
    return 0;
}

---

Update: I will leave the code I originally posted above. Any modifications that I make will be posted below.

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

#define NALPHA (26) 

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

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

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

    if (!fp) {
        fprintf(stderr, "Error: Could not open file: %s\n", argv[1]); 
        return 1; 
    }

    while ((ec = fgetc(fp)) != EOF) {       
        if (isalpha(ec)) { 
            const int ascii_offset = islower (ec) ? 'a' : 'A';
            int wrap = (ec + offset - ascii_offset) % NALPHA;
            if (wrap < 0) {
                wrap += NALPHA ;
            }
            ec = wrap + ascii_offset;       
        }

        fputc(ec, stdout);
    }

    fclose(fp);
    return 0;
}

Answer 1

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;
  

Answer 2

1. Macros in C are typically named in UPPERCASE.

2. You can reduce some code duplication in this block:

       if(islower(ec)) {
           wrap = (ec + offset - lstart) % nalpha;
           wrap = (wrap < 0) ? nalpha + wrap : wrap; 
   
       } else {
           wrap = (ec + offset - ustart) % nalpha;
           wrap = (wrap < 0) ? nalpha + wrap : wrap; 
       }
   
       ec = islower(ec) ? wrap + lstart : wrap + ustart;
   

   The only difference in the different branches are lstart vs ustart. You can store it in a variable first and use that generically:

       int ascii_offset = islower(ec) ? lstart : ustart;
   
       wrap = (ec + offset - ascii_offset) % nalpha;
       wrap = (wrap < 0) ? nalpha + wrap : wrap; 
   
       ec = wrap + ascii_offset;
   

3. Changing the input file seems dodgy to me. The Unix way would be to write the "encrypted" output to stdout. This way the user can choose what to do with it (redirect to another file or to another program for example).

Answer 3

• Putting parens around macro constants like USTART (0x41) is a good idea. Suggest doing it with all as in NALPHA (26).

• To be highly portable, the assumption A to Z are continuous need to be abandoned. But I see marginal value in accommodating things like EDCIDC.

• Minor: Robust would check return values of fseek(), fputc(), and fclose().

• Better error message:

  fprintf(stderr, "Error: could not open file: \"%s\"\n", argv[1])