I'm a C++ programmer. I've never written a C program before. But most of what I say should be applicable.
Globals
int i, c;
char *key;
There's no reason for these variables to be global. These variables should be declared in the same place they are used.
These variables also have very bad names. What is i
? What is c
? I'll have to read the rest of the code to find out.
String typedef
int main(int argc, string argv[])
I don't think using string
is a good idea. Somewhere in that cs50 header there's probably
typedef char *string;
When you apply const
to this typedef
, you don't get what you expect. You may expect that const string
is the same as const char *
, but it isn't. const string
is the same as char *const
. You should use a pointer to a character when you want a pointer to a character.
Declare variables as late as possible
int int_A = (int) 'A';
int int_a = (int) 'a';
When you're declaring your variables, you should leave it to the last minute. If you don't need the variables yet, don't declare them.
Conditions
if (argc != 2)
{
// The wrong number of arguments
}
else if (argc == 2)
{
// The right number of arguments
}
If the condition argc != 2
is false
, we know that the condition argc == 2
must be true
so there is no need to confirm that it is true
. Also, when there is the wrong number of arguments, the function returns. Your if
should become this:
if (argc != 2)
{
// The wrong number of arguments
}
// The right number of arguments
Unhelpful error messages
When the user doesn't provide exactly 1 argument, this happens:
printf("Please use a single alphabetic command line argument\n");
return 1;
When the user provides a single argument that does not consist of only alphabetic characters, this happens:
printf("Please use a single alphabetic command line argument\n");
return 1;
You should print two different error messages for two different problems. Perhaps "Please provide a single argument" and "Please provide an alphabetic string"
Bad variable names
for (int z = 0, n = strlen(argv[1]); z < n; z++)
{
if (isalpha(argv[1][z]) == false)
{
printf("Please use a single alphabetic command line argument\n");
return 1;
}
}
Here, you're referring to the key as argv[1]
. At this point, it might make more sense to say "the key must be an alphabetic string" rather than "the first command-line argument must be an alphabetic string". You're also calculating the size of the key twice. You're doing it here in the for
loop, then right after it
// foo bar mitzva
key = argv[1];
int key_len = strlen(argv[1]);
int key_i[key_len];
We've found the place where key
is used. You should put this snippet before the for
loop and have the for
loop refer to key
rather than argv[1]
.
Strings are pointers, pointers can move
The part of your code that makes sure the key is an alphabetic string can be made more efficient. Your code finds the length of the key twice and then checks to see if every character is alphabetic. This is a whopping 3 iterations when only one is necessary.
char *key = argv[1];
for (const char *k = key; *k != 0; ++k)
{
if (!isalpha(*k))
{
printf("The cipher key must be an alphabetic string\n");
return 1;
}
}
Is this a heap allocation?
// get users plaintext
printf("Plaintext: ");
char* plaintext = get_string();
I'm not sure if get_string
allocates memory for the string or returns a pointer to static memory. You might want to specify that in a comment. If plaintext
is heap allocated, you should call free(plaintext)
to free the memory when you're done with it (at the very end of the program).
Characters are numbers
for (i = 0; i < key_len; i++)
{
if (isupper(key[i]))
{
key_i[i] = (argv[1][i] - int_A);
}
if (islower(key[i]))
{
key_i[i] = (argv[1][i] - int_a);
}
We've finally found the place where that global i
is used. You should declare variables right when you need them. I'm pretty sure you're missing a }
at the end of this for
loop.
Here, you're copying the key into an array of ints. There is a bit of duplicate work here. If you know that a character is alphabetic, then it can only be upper case or lower case. If you know the character is upper case, then you know that it cannot be lower case. You could actually do this in the same place that you are checking you're characters are alphabetic. What I'm saying is:
char *key = argv[1];
const int key_len = strlen(key);
//If this was C++, I would definitely perform a heap allocation but
//heap allocation is a pain in C so I'll just use this C99 trick.
int key_i[key_len];
for (int idx = 0; idx != key_len; ++idx)
{
const char character = key[idx];
if (islower(character))
{
// characters are numbers, remember?
key_i[idx] = character - 'a';
}
else if (isupper(character))
}
key_i[idx] = character - 'A';
}
else
{
//If the character is not lower case, and not upper case, then its not alphabetic
printf("The cipher key must be an alphabetic string\n");
return 1;
}
}
The actual cipher part of the cipher program
Alright, now I'll find out what a Vigenere cipher is!
// set i back to 0 for the next loop
i = 0;
// iterate over characters in plaintext and shift plaintext character by key
printf("ciphertext: ");
for (int j = 0, n = strlen(plaintext); j < n; j++)
Use proper variable names. What is i
? What is j
? You should rename them to key_idx
and plaintext_idx
respectivly.
Conditionals (again)
int pt = (int) plaintext[j];
if (isalpha(plaintext[j]))
{
if (isupper(plaintext[j]))
{
printf("%c", (((pt - int_A) + key_i[i % key_len]) % 26) + int_A);
}
if (islower(plaintext[j]))
{
printf("%c", (((pt - int_a) + key_i[i % key_len]) % 26) + int_a);
}
}
else
{
printf("%c", plaintext[j]);
}
if (isalpha(plaintext[j]))
{
i += 1;
}
pt
never changes so it should be declared const
. There's no need to repeatedly index the plaintext
when you already have pt
just lying around, not doing much. Try not to pack a whole load of math into one line. If you know that an alphabetic character is not upper case then it must be lower case. This snippet could be simplified to this:
const int pt = plaintext[plaintext_idx];
const int key_char = key_i[key_idx % key_len];
if (isupper(pt))
{
// most of those brackets are unnecessary.
printf("%c", (pt - 'A' + key_char) % 26 + 'A');
key_idx += 1;
}
else if (islower(pt))
{
printf("%c", (pt - 'a' + key_char) % 26 + 'a');
key_idx += 1;
}
else
{
printf("%c", pt);
}
Characters, aren't they weird
Fun fact about C, characters literals aren't actually char
s, they're int
s! In a lot of places when C expects a "character", what it actually expects is an int
. isupper
and islower
expect int
s. printf("%c", integer)
expects an int
. Just keep that in mind when handling strings in C.
Some more optimization
This program could probably be made much smaller, faster and neater (key_i
can be removed) but it's 22:00 in Australia and I have school tomorrow so that's all for now.