What an Enigma! (Machine Simulator)

Question

I've made this some time ago for an assignment and I was wondering how it could be improved both in terms of performance and best practices for C code.

#include <stdio.h>
#include <string.h>

#define NR_ROTORS 3   /* Number of rotors */
#define ROTORSIZE 26   /* Rotor charset size */
#define INIT_CHAR 'A'  /* initial char */

typedef struct Rotor Rotor;

struct Rotor {
    int start;   
    int rotatePos;  
    int rotateCount; 
    char elemts[ROTORSIZE+1]; 
};

void rotorInit(Rotor *r); 
void reflectorInit(int *reflector, char *conf);
int getConf(Rotor *r, int *reflector, int *messages);
int calcOffset(Rotor r, int offset);
void shiftRotors(Rotor *r);
int encrypt(Rotor *r, int *reflector, int rotate, int offset);
int mod(int a, int b);

void rotorInit(Rotor *r){
    for(int i = 0; i < NR_ROTORS; i++) {
        r[i].start = 0;
        r[i].rotatePos = 0;
        r[i].rotateCount = 0;   
    }
}

void reflectorInit(int *reflector, char *conf) {
    for(int i = 0; i < ROTORSIZE; i++) {
        for(int j = 0; j < ROTORSIZE; j++) {
            if (conf[i] == conf[j] && reflector[i] == -1 && i!=j) {
                reflector[i] = j;
                reflector[j] = i;
                break;
            }
        }
    }
}

int getConf(Rotor *r, int *reflector, int *messages) {
    for(int i = 0; i < NR_ROTORS; i++) {
        scanf("%s %d", r[i].elemts , &r[i].rotatePos);
        if (strlen(r[i].elemts) != ROTORSIZE)
            return 1;
    }
    char conf[ROTORSIZE+1];
    scanf("%s", conf);
    reflectorInit(reflector, conf);
    scanf("%d", messages);
    if (strlen(conf) != ROTORSIZE || *messages < 0)
            return 1;
    return 0;
}

int calcOffset(Rotor r, int offset) {
    return mod(offset + r.elemts[mod((r.start+offset),ROTORSIZE)] - 
           (INIT_CHAR + mod((r.start+offset),ROTORSIZE)),ROTORSIZE);    
}

void shiftRotors(Rotor *r) {
    r[NR_ROTORS-1].start = mod(++r[NR_ROTORS-1].start, ROTORSIZE);
    r[NR_ROTORS-1].rotateCount++;
    for (int i = NR_ROTORS-1; i > 0; i--) {
        if (r[i].rotateCount == ROTORSIZE) {
            r[i-1].start = mod(++r[i-1].start, ROTORSIZE);
            r[i-1].rotateCount++;
            r[i].rotateCount = 0;
        }
    }
}

int encrypt(Rotor *r, int *reflector, int rotate, int offset) {
    for (int j = NR_ROTORS-1; j >= 0; j--)
        offset = calcOffset(r[j], offset);
    offset = reflector[offset];
    for (int j = 0; j < NR_ROTORS; j++) {
        for (int k = 0; k < ROTORSIZE; k++) {
            if (INIT_CHAR + (mod((r[j].start+(offset)),ROTORSIZE))
                == r[j].elemts[mod((r[j].start+k),ROTORSIZE)]) {
                offset = k;
                break;
            }
        }
    }
    return offset;
}

int mod(int a, int b) {
    int r = a % b;
    return r < 0 ? r + b : r;
}

int main (int argc , char* argv[]) {
    Rotor rotors[NR_ROTORS];
    int reflector[ROTORSIZE] = {[0 ... ROTORSIZE-1] = -1};
    char initPos[NR_ROTORS+1];
    int messages = 0, rotate = 0;
    rotorInit(rotors);
    if (getConf(rotors, reflector, &messages) == 1) {
        fprintf (stderr, "Error: Bad input\n");
        return 1;
    }
    for (int i = 0; i < messages; i++) {
        initPos[NR_ROTORS] = 0;
        scanf("%s\n", initPos);
        if (initPos[NR_ROTORS] != '*')
            rotate = 1;
        else rotate = 0;
        for(int j = 0; j < NR_ROTORS; j++) {
            rotors[j].start = initPos[j] - INIT_CHAR;
            rotors[j].rotateCount = ROTORSIZE - 
                (rotors[j].rotatePos - rotors[j].start+1);
            rotors[j].rotateCount = mod(rotors[j].rotateCount, 
                            ROTORSIZE);
        }
        int offset = 0;
        char c = fgetc(stdin);
        while (c != '\n') {
            if (rotate == 1) 
                shiftRotors(rotors);
            offset = c - INIT_CHAR; 
            printf("%c",INIT_CHAR
                + encrypt(rotors, reflector, rotate, offset));
            c = fgetc(stdin);
        }
        printf("\n");
    }
    return 0;
}

In order to run the program you need to give it an initial rotor and reflector configurations. Input example:

EKMFLGDQVZNTOWYHXUSPAIBRCJ 16
AJDKSIRUXBLHWTMCQGZNPYFVOE 4
BDFHJLCPRTXVZNYEIWGAKMUSQO 21
ABCDEFGDIJKGMKMIEBFTCVVJAT
2
MCK
QMJIDOMZWZJFJR
ABC
ENCRYPTTHISMESSAGE

• The first 3 lines define the rotor configuration. The integer denotes the position in which the rotor causes the rotation of next rotor.
• Fourth line defines the reflector configuration.
• The fifth line is the number of messages you want to encrypt/decrypt
• Sixth line is the initial position (letter) of each rotor
• Seventh line is the actual message you want to encrypt/decrypt

Repeat six and seven depending on your input in the fifth line.

Answer 1

Basic program design

• Proper program design would be to put the Rotor struct in a module of its own, consisting of rotor.h (caller's interface) and rotor.c (implementation).
• You should name all your functions dealing with the Rotor with a common prefix, so that you indicate to the caller what their purpose is and which module they belong to. For example rotorInit, rotorGetConf and so on. You can use a similar prefix for constants/macros belonging to the same module.
• Try to separate algorithms from user interface. That is, leave all input/output functions to the caller, or to a separate GUI module.

Advanced program design

• The professional way of designing programs involve object-oriented concepts, even in C. Apart from the OO concepts modular design and autonomous code which I already mentioned above, you should also strive for private encapsulation. The caller should not concern themselves with the internals of the struct, internal helper functions used by your module etc. In C, this is achieved with two language features:
  • All local functions and local variables should be declared as static to reduce their scope. (Though be aware the local file scope variables should be avoided if multi-threading is used.)
  • The implementation of the struct should be completely hidden to the caller by the concept opaque type, which involves declaring a struct of incomplete type in the header, which is then only defined in the .c file.

Best practices

• All functions that do not modify the passed Rotor object should declare it as a pointer to constant data const Rotor*. Look up the term const correctness.
• As already mentioned in the other answers, make sure to check the result of library function calls and if programming desktop programs, avoid functions that make your program vulnerable to buffer overflows. Most of stdio.h is unsuitable for production-quality code, save for fgets which is the recommended function to use for all user input.

Answer 2

Comment on Style

The problem I have with C programs is that people continuously fail to check if there system calls worked. You should always check the result of a system call.

This is doubly true when getting user input. Users a re dumb stupid animals and will do the most random things. You should program defensively and validate all user input.

Example: // System call and user input

scanf("%s %d", r[i].elemts , &r[i].rotatePos);

Couple of problems here.

• The string could overflow r[i].elemts
• What happens of the string has a space
• What happens if the number is not actually a number

All these things should be validated.

#define ROTORSIZE 26
// You can generate the next value using some macro magic
// for this review I am being lazy and just specifying it.
#define ROTORSCANSTRING  "%26s"

// Adding the "%c" to make sure the next character was a space
// Otherwise the input string was two long.
char chk;
if (scanf(ROTORSCANSTRING "%c" " %d", r[i].elemts , &chk, &r[i].rotatePos) != 3) {
     exit(1); // failed to read all the inputs.
}
if (!is_space(chk) || strlen(r[i].elemts) != ROTORSIZE) {
     exit(1); // The string was larger/smaller than ROTORSIZE
}
if (r[i].rotatePos < 0 || r[i].rotatePos >= 26) {
     exit(1); // Invalid position.
}

Code Review

Why not combine the following into a single statement?

typedef struct Rotor Rotor;

struct Rotor {
    int start;   
    int rotatePos;  
    int rotateCount; 
    char elemts[ROTORSIZE+1]; 
};

I would have done:

typedef struct Rotor {
    int start;   
    int rotatePos;  
    int rotateCount; 
    char elemts[ROTORSIZE+1]; 
} Rotor;

The code is pretty dense. I assume it works but its to hard to work out. It would be nice to have explaining comments on how it works.

Answer 3

Dangerous scanf()

This code here is susceptible to buffer overruns:

char conf[ROTORSIZE+1];
scanf("%s", conf);

If the user inputs a string longer than ROTORSIZE (26), your buffer will overflow and your program may crash.

One way of avoiding the buffer overruns is to use fgets instead. You could use fgets to grab the whole line, and then you can use sscanf to parse the rotor part of the line, knowing that the line length is bounded:

#define LINESIZE 80  // Or whatever length you prefer

char line[LINESIZE];
char conf[LINESIZE];

if (fgets(line, sizeof(line), stdin) == NULL)
    return 1;
if (sscanf(line, "%s", conf) != 1)
    return 1;
if (strlen(conf) != ROTORSIZE)
    return 1;
// ...