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);
    else if (islower(message[i])) {
      char d = (message[i] - 97 + cipher[keyCount]) % 26 + 97;
      printf("%c", d);
    else {
      printf("%c\n", message[i]);
    if (keyCount >= p) {
      keyCount = 0;

i++ or ++i?

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


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?


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.

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);

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.

  • \$\begingroup\$ Thanks, some food for thought here, I will go over it fully once I get home from work. \$\endgroup\$ – Adam 'Sacki' Sackfield Sep 8 '15 at 9:11


  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;


  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++) {


  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');


      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.

  • \$\begingroup\$ Thanks, I agree with the changes although subtle make it easier to read, I will go over it fully once I arrive home. \$\endgroup\$ – Adam 'Sacki' Sackfield Sep 8 '15 at 9:14

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.