patterncMinor
Vigenere's Cipher in C
Viewed 0 times
vigenerecipherstackoverflow
Problem
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
#include
#include
#include
#include
// Function to check if alphabet characters.
int alphaCheck(char * argv[]) {
int length = strlen(argv[1]), n;
for (n = 0; n = p) {
keyCount = 0;
}
}
}Solution
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 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.Code Snippets
int main(int argc, char* argv[])int alphaCheck(char *str) {
for ( ;*str; ++str) {
if (!isalpha(*str)) {
...
}
}
return 0; // You don't have this return.
}char* secret = keyWord[1];for each character c in the message:
let e = encrypt c using the secret key
printf("%c", e)
advance pointers, counters, etc.
printf("\n")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");Context
StackExchange Code Review Q#102737, answer score: 2
Revisions (0)
No revisions yet.