[pset2] - Vigenere.c

It's definitely not a mess but a few improvements/optimizations could be made.


Line 15: The parenthesis are not necessary around argv[1].

Line 17: Avoid using strlen in a for-loop like that. You are calculating the length of k on every iteration of that loop which is inefficient. Store the output of strlen(k) in a variable before the loop and use that instead.

Line 19: I don't think you meant to use a bitwise OR here, but because boolean expressions evaluate to 1 or 0, it happens to work here. For future reference, unless you are meaning to be doing bitwise operations (you'll know when you are), || is the symbol you want to be using for "or".

Line 19 (continued): You should be using isalpha() here anyway.

Line 26: Again, avoid using strlen in a loop if possible.

Line 30: See previous suggestion.

Line 31-34: This could be made slightly more elegant. Look into the modular division operator (%)

Line 35 and 40: Lowercase and uppercase letters are mutually exclusive (meaning there doesn't exist a letter that is both upper and lower case. For this reason, you should be using an if/else expression here rather than two seperate if statements. This way if that first if statement is satisfied (letter is lower case), you don't have to check if it is uppercase on line 40 (because this will always be false).

Lines 35-44: You are using a lot of "magic" numbers here that could make your code hard to read if you're not familiar with the ASCII character codes off the top of your head. Consider replacing 97 with 'a' and 65 with 'A'.

Lines 35-44: Whenever you see code duplication like this, it's a good idea to reexamine your method. You have two nearly identical blocks of code: the only difference being the case. Here's what I would do:

int c;
if (islower(plaintext[i]))
    c = 'a';
else
    c = 'A';

printf("%c", (((plaintext[i] - c) + (tolower(k[j]) - 'a')) % 26) + c));

You could even shorten my example a little more by replacing the first 5 lines with the ternary operator: int c = (islower(plaintext[i])) ? 'a' : 'A' but this is much less readable.


Overall, very well done! As a general critique I'd work on your indentation a bit but as I said, the program is good as is.

/r/cs50 Thread Parent