10
\$\begingroup\$

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;
}
\$\endgroup\$
6
\$\begingroup\$

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;
    
\$\endgroup\$
  • \$\begingroup\$ 1. Should I even bother with having the LSTART and USTART macros? 2. The C89/90 ISO forbids mixing variable declarations and code. I do not want to use a newer version of C. 3. Good catch on the fputc. I'll post my updated code in a little bit. \$\endgroup\$ – red_eight Dec 16 '13 at 0:37
  • \$\begingroup\$ I'd like to make offset a const int but I'm not sure if that's possible since I have to do the argument count check first. \$\endgroup\$ – red_eight Dec 16 '13 at 0:57
  • \$\begingroup\$ 1. Most reviewers advise against embedding magic values because such values are difficult to find and change, especially if used more than once. That doesn't mean you should never do it, just that it is best to have a good reason for doing so. In this case, as the two are used just once, it might even be more readable to use 'a' and 'A', as they are unlikely ever to change. And the names LSTART and USTART do not have obvious meanings (I should have commented on that). On the other hand, NALPHA is used twice and is probably best kept as a macro. \$\endgroup\$ – William Morris Dec 16 '13 at 1:59
  • \$\begingroup\$ 2. Why would you want to stick with an outdated version of C? I think the changes introduced in C99 improve the language. \$\endgroup\$ – William Morris Dec 16 '13 at 2:01
  • \$\begingroup\$ Even though I wrote this on Linux, I use Visual Studio on my Windows machine. As far as I know, they don't really like C99. \$\endgroup\$ – red_eight Dec 16 '13 at 2:25
5
\$\begingroup\$
  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).

\$\endgroup\$
  • 1
    \$\begingroup\$ I have made the suggested changes. I'm thinking that the LSTART and USTART macros might be a little pointless. They don't really seem to help with readability. I originally had them for Unicode support, but I don't think functions such as isalpha and islower are Unicode friendly. Do you think it would be better to just substitue 'a' for LSTART and 'A' for USTART? \$\endgroup\$ – red_eight Dec 14 '13 at 23:21
2
\$\begingroup\$
  • 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])
    
\$\endgroup\$
  • \$\begingroup\$ Abandoning that assumption would break my algorithm. I'll read up on EDCIDC, but most likely I won't be able to support it. Good call on the error message. \$\endgroup\$ – red_eight Dec 16 '13 at 0:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.