Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
Source Link
ErikR
  • 1.9k
  • 10
  • 7

i++ or ++i?

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

http://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.

lang-c

AltStyle によって変換されたページ (->オリジナル) /