Vigenere's Cipher in C

Question

I have updated the code and the program now works as it should. If you could critique my code that would be great. I am new to C so I guess I will have made some rookie error :)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <stdbool.h>
// Function to check if alphabet characters.
int alphaCheck(char * argv[]) {
 int length = strlen(argv[1]), n;
 for (n = 0; n < length; n++) {
 if (!isalpha(argv[1][n])) {
 printf("Characters 'A-Z' for Keyword.\n");
 return 2;
 }
 }
}
int main(int argc, char * keyWord[]) {
 int cipher[64], i, j, k;
 char message[128];
 int keyCount = 0;
 int p = strlen(keyWord[1]);
 // Validation - using alphaCheck Function
 if (argc != 2 || alphaCheck(keyWord) == 2) {
 return 0;
 }
 // For loop to convert to upper and set value ie A = 0 B = 1
 for (i = 0, j = strlen(keyWord[1]); i < j; i++) {
 cipher[i] = (toupper(keyWord[1][i]) - 65);
 }
 // Prompt the user for the message to encrypt
 printf("Enter your secret message: ");
 fgets(message, 128, stdin);
 for (i = 0, k = strlen(message); i < k; i++) {
 if (isupper(message[i])) {
 char c = (message[i] - 65 + cipher[keyCount]) % 26 + 65;
 printf("%c", c);
 keyCount++;
 }
 else if (islower(message[i])) {
 char d = (message[i] - 97 + cipher[keyCount]) % 26 + 97;
 printf("%c", d);
 keyCount++;
 }
 else {
 printf("%c\n", message[i]);
 }
 if (keyCount >= p) {
 keyCount = 0;
 }
 }
}

Answer 1

i++ or ++i?

Using i++ is fine here, but also read this discussion:

https://stackoverflow.com/questions/4261708/i-or-i-in-for-loops

fgets(message, 128, stdin);

How about fget(message, sizeof(message), stdin) -- makes it easier to change the size of the message buffer.

int main(int argc, char * keyWord[])

Most people are used to seeing:

int main(int argc, char* argv[])

int alphaCheck(char * argv[])

How about:

int alphaCheck(char *str) {
 for ( ;*str; ++str) {
 if (!isalpha(*str)) {
 ...
 }
 }
 return 0; // You don't have this return.
}

First of all, this makes alphaCheck usable on any string, not just argv[1]. Also you avoid computing the length of the string. Moreover, this is the idiomatic way to iterate over a null-terminated string in C.

Finally - make sure you always return a value since you've declared this function returns an int.

for (i = 0, j = strlen(keyWord[1]); i < j; i++) {

You are modifying cipher[i] but are you checking that i < 64? What if keyWord[1] has length> 64?

keyWord[1]

You reference this array element many times in the program. Perhaps you should assign a meaningful variable name to it, e.g.:

char* secret = keyWord[1];

It will make you program more readable.

printf("%c\n", message[i]);

Do you want to always print a \n here? I would think you only want to print an extra \n at the end of the message.

the main loop

You main loop has this structure:

for each character c in the message:
 let e = encrypt c using the secret key
 printf("%c", e)
 advance pointers, counters, etc.
printf("\n")

so it would be nice if the code could match this as much as possible.

How about:

int i;
for (char *p = message, i = 0; *p; ++p, ++i) {
 char e = ...encrypt (*p) using secret, strlen(secret) and i...
 printf("%c", e);
}
printf("\n");

Now there is only one place where the encrypted character is printed out - instead of three difference places.

You can implement this via a helper function:

char encrypt(char* secret, int secret_len, char c, int i) {
 // return how c is encrypted when at position i in the message.
 if (isupper(c)) {
 return ...
 } else if (islower(c)) {
 return ...
 } else {
 return c;
 }
}

Hint: The expression i % secret_len might come in handy here.

• Adam 'Sacki' Sackfield: Thanks, some food for thought here, I will go over it fully once I get home from work.

Answer 2

Declarations

 int length = strlen(argv[1]), n;

In general, it's preferable to put only one declaration in a line where you do an assignment. So either say

 int length = strlen(argv[1]);
 int n;

or

 int length, n;
 length = strlen(argv[1]);

Either way can work. I like the first one better. The only thing that I'd say is that I like having the declaration and the initialization on the same line. I don't find the declarations to be tied to each other as much as the initialization is tied to the declaration.

Naming Conventions

You have

 for (n = 0; n < length; n++) {

and

 for (i = 0, j = strlen(keyWord[1]); i < j; i++) {

As a convention, i, j, and k are index variables and n, m, and l are limit variables. So the first line above would use i instead of n and the second would become

 for (i = 0, n = strlen(keyWord[1]); i < n; i++) {

Note that this is purely a convention. There's no functional reason for this.

Magic Numbers

 char message[128];

Avoid repeated uses of bare numbers (often called magic numbers). Instead define constants. The original C way was

#define MESSAGE_LENGTH 128

which you'd use

 char message[MESSAGE_LENGTH];

This way you can change the value in just one place and have it affect many places.

Note that modern C compilers will generally handle the same syntax as C++ uses for constants. I'm less familiar with that though.

 cipher[i] = (toupper(keyWord[1][i]) - 65);

I would consider this a special case of a magic number. But you don't need to define a constant for it. Just say

 cipher[i] = (toupper(keyWord[1][i]) - 'A');

Later,

 char d = (message[i] - 97 + cipher[keyCount]) % 26 + 97;

can become

 char d = (message[i] - 'a' + cipher[keyCount]) % ALPHABET_SIZE + 'a';

Note that you could argue that 26 will never change. But another reason to define constants is that they act as comments. Now I don't have to think about what 26 means or look for a comment. The formula reads more naturally with the constant.

• Adam 'Sacki' Sackfield: Thanks, I agree with the changes although subtle make it easier to read, I will go over it fully once I arrive home.