C Vigenere Encryptor

Question

This code is tested and working properly, I'm looking for some feedback on the following functions. Pretty straight forward and simple, but if additional information is desired, ask and I will gladly oblige. Comments, suggestions, etc are welcomed.

The following methods are the functional portion of a program that serves as a Vigenere encryption application.

The removeDuplicates function serves to remove duplicates from the user-entered key phrase, e.g. if infinitestring was passed in inftesrg would be returned.

targetFound serves as a helper for identifying duplicates present as removeDuplicates builds the final key. I realize it is not completely necessary, but the prototypes were supplied in the project description, and while I did change the return-types and parameters passed, I still decided to use all of the functions specified.

initializeEncryptArray builds the encryption substitution table, initializeDecryptArray builds the decryption substitution table, however it dependent on initializeEncryptArray as the parameters describe. The idea is shown in the following:

key: feather

cipher:
A|B|C|D|E|F|G|H|I|J|K|L|M|N|O|P|Q|R|S|T|U|V|W|X|Y|Z
⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ ⇵ 
F|E|A|T|H|R|Z|Y|X|W|V|U|S|Q|P|O|N|M|L|K|J|I|G|D|C|B

processInput processes the input file specified by the user, and writes to the output file.

Essentially in main I parse arguments given at command line, for encrypt/decrypt (e or d) options, an input file, and an output file, in the following fashion ./a.out e <key-word> <input-file-name> <output-file-name> or ./a.out d <key-word> <input-file-name> <output-file-name>. For the encrypt option, the key is passed to removeDuplicates to remove duplicate characters. The key (less the duplicate characters) is then passed to initializeEncryptArray and the rest of the alphabet is appended in reverse order to create the substitution table. The substitution table is then passed to processInput along with references to the input and output files.

The same sequence happens for the decrypt option, except for after initializeEncryptArray the result is passed to initializeDecryptArray instead of processInput and the resulting table from initializeDecryptArray is then passed to processInput along with the references to the input/output files.

p1.c:

#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include "functions.h"


int main(int argc, char * argv[]){
  int MAXNUM = 27;
  char * encrypt, * decrypt, * key;
  char choice;
  FILE * fin, * fout;

  //check argument count
  if (argc != 5)
    errorHandler(0);

  choice = toupper(*argv[1]);
  key = removeDuplicates(argv[2]);

  fin = fopen(argv[3], "r");
  fout = fopen(argv[4],"w");

  if(fin == NULL || fout == NULL)
    errorHandler(3);

  encrypt = (char   *) malloc (MAXNUM * sizeof(char));

  //allocation check
  if (encrypt == NULL)
    errorHandler(2);

  //encrypt option
  if (choice == 'E'){
    initializeEncryptArray(key, encrypt);
    processInput(encrypt, fin, fout);
  }
  //decrypt option
  else if (choice == 'D'){
    decrypt = (char *)  malloc (MAXNUM * sizeof(char));

    if(decrypt == NULL)
      errorHandler(2);
    initializeEncryptArray(key, encrypt);
    initializeDecryptArray(encrypt, decrypt);
    processInput(decrypt, fin, fout);
    free(decrypt);
  }
  //invalid choice
  else
    errorHandler(1);

  free(encrypt);
  fclose(fin);
  fclose(fout);
}

functions.c:

 #include "functions.h"

// remove duplicate characters in array word and return the resulting string
char * removeDuplicates(char * word){
  char * result;
  int len = strlen(word) + 1;
  result = malloc (len);

  if (result == NULL){
    errorHandler(2);
  }

  char ch;

  for(int i = 0, j = 0; i < len; i++){
    ch = word[i];

    if(!targetFound(result, ch)){
      result[j] = ch;
      j++;
    }
    result[j] = '\0';
  }

  return result;
}

// search the first num characters in array charArray for character target
// return a non-zero integer if found, otherwise, return 0
int targetFound(char * charArray, char target){
  int found = 0;

  if(strchr(charArray,target))
    found = 1;

  return found;
}

// initialize the encrypt array with appropriate cipher letters according to the given key
void initializeEncryptArray(char * key, char * encrypt){
  char ch;

  for (int i = 0, j = 0, flag = 0; j < 27;){
    ch = key[i];

    if (ch == '\0' || flag == 1){
      if(flag == 0)
        flag = 1;

      ch = 'Z' - j;
      j++;
    }

    if (strchr(encrypt,ch) == NULL){
      encrypt[i] = toupper(ch);
      i++;
    }
  }
  encrypt[26] = '\0';
  printf("Encrypt: %s\n\n", encrypt);
}

// initialize the decrypt array with appropriate cipher letters according to the given key
void initializeDecryptArray(char * encrypt, char * decrypt){
  int i;
  for ( i = 0; i < 27; i++){
    decrypt[encrypt[i] - 'A'] = i + 65;
  }
  decrypt[26] = '\0';
  printf("Decrypt: %s\n\n", decrypt);
}

// process data from the input file and write the result to the output file
// pass the encrypt array to parameter substitute if encryption is intended
// pass the decrypt array to parameter substitute if decryption is intended
void processInput(char * substitute, FILE * fin, FILE * fout){
  char ch;
  while ( fscanf(fin, "%c", &ch) != EOF ){
    if(isupper(ch)){
      fprintf(fout, "%c", substitute[ch - 'A']);
    }
    else if(islower(ch)){
      fprintf(fout, "%c", tolower(substitute[ch - 'a']));
    }
    else
      fprintf(fout, "%c", ch);
    }
}

// prints errors based on the error code passed in
void errorHandler(int argIndex){
  switch(argIndex){
    case 0:
      printf("Error: Incorrect number of arguments received.\n\tPlease try again using the correct number of args.\n");
    case 1:
      printf("Error: Invalid program option.\n\tPlease try again using either 'd'/'D' or 'e'/'E'.\n");
    case 2:
      printf("Error: Memory could not be allocated.\n");
    case 3:
      printf("Error: Invalid input filename given for input.\n\tPlease use a valid filename for input.\n");
  }
  exit(1);
}

functions.h

#ifndef FUNCTIONS_H
#define FUNCTIONS_H

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <string.h>

// remove duplicate characters in array word and return the resulting string
char * removeDuplicates(char * word);

// search the first num characters in array charArray for character target
// return a non-zero integer if found, otherwise, return 0
int targetFound(char * charArray, char target);

// initialize the encrypt array with appropriate cipher letters according to the given key
void initializeEncryptArray(char * key, char * encrypt);

// initialize decrypt array with appropriate substitute letters based on the encrypt array
void initializeDecryptArray(char * encrypt, char * decrypt);

// process data from the input file and write the result to the output file
// pass the encrypt array to parameter substitute if encryption is intended
// pass the decrypt array to parameter substitute if decryption is intended
void processInput(char * substitute, FILE * inf, FILE * outf);

//prints out user-friendly error message
void errorHandler(int errIndex);

#endif //FUNCTIONS_H

Answer 1

Here are some things that may help you improve your program.

Fix the bugs part 1

The first problem my compiler noticed is that you are using '/0' in a few places as though it were equivalent to '\0' which is the NUL character. It isn't the same! What you're defining is a multibyte character constant which, if it compiles, certainly won't be what you intended.

Fix the bugs part 2

The removeDuplicates() function allocates memory for result but that memory is never freed and it's also used before it's initialized -- it's used as though it has a termminating NUL char but none has been inserted by your code. There's a similar error in the initializeEncryptArray routine and several other places in which uninitialized data is used. Although the code inserts a NUL at the end of the routine, it is passing the string to strchr() before that happens, which can lead to undefined behavior.

Provide break for each case

Your error handler includes code like this:

switch(argIndex){
  case 0:
    printf("Error: Incorrect number of arguments received.\n\tPlease try again using the correct number of args.\n");
  case 1:

The problem is that you are missing a break statement at the end of each case. This suggests that you haven't tested the program very much, which makes me think you need to spend more time on it before presenting it for review.

Think of the user

Assuming the two errors above are addressed, if the user runs the program with no arguments, the error message is this:

Error: Incorrect number of arguments received.
        Please try again using the correct number of args.

That's not very helpful. What is the correct number of arguments and what do they signify? Generally, an error message should say what is wrong, but also, if possible, give some hint to the user about how to fix it.

Use const where practical

The MAXNUM variable is apparently the size of the encryption array and does not change. For that reason, I'd suggest that it should be declared const int EncryptionArraySize = 27; instead. The use of all uppercase for an identifier traditionally has identified a macro, which this is not.

Simplify your code

The code currently includes this line:

encrypt = (char *) malloc (MAXNUM * sizeof(char));

There are a few things I'd change there, in addition to the name of MAXNUM as mentioned above. I'd write it like this:

encrypt = malloc(MAXNUM); 

First, malloc does not typically require a cast. Second, sizeof(char) is defined by the standard to always be equal to one, so there's really no need for that here.

Think carefully about memory allocation

Because the encrypt and decrypt arrays are quite short (27 chars), I'd probably suggest allocating them as automatic variables (that is, stack variables) eliminating the need for explicit allocation/free and associated error handling. Also, the removeDuplicates memory allocation for result could easily be eliminated by doing the removal in-place:

char * removeDuplicates(char * word) {
    for (char *ch = word; *ch; ) {
        char *last = strrchr(ch, *ch);
        if (last == ch) {
            ++ch;
            continue;
        }
        for ( ; *last; ++last) {
            *last = last[1];
        }
    }
    return word;
}

Prefer switch to if..else chain

In main there is a longish if..else chain that examines the users choice of encrypt or decrypt and acts appropriately. That may be more cleanly described as a switch with a default case to handle the error.

Rethink the interface

Right now, the main code defines the length of the key array using a named constant MAXNUM, which is good. However, this length is then assumed within the initializeEncryptArray and initializeDecryptArray. Either pass the parameter or use the fact that it's a NUL terminated string (e.g. use strlen()). Also, I'd suggest the removeDuplicates and initializeEncryptArray could easily be combined.