Here are some observations that may help you improve this code.
Don't use gets
Using gets
is not good practice because it can lead to buffer overruns. It has been removed from the C11 standard and marked "obsolete" in POSIX 2008. Use fgets
instead, or in this case, you could simply pass in the string as a command line parameter. Alternatively, you could have the number passed in as a command line option and the string could be read from stdin
which would allow use of the program as a pipe.
Check return values for errors
The call to scanf
can fail. You can check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.
Consinder signed versus unsigned
The variable rotatorN
is declared as an int
which is a signed quantity and the scanf
function will allow a user to enter a negative number such as -99. However, the program does not produce useful output in that case, so it would be better to restrict the value to positive numbers. In fact, it would probably make sense to restrict it to the range of 0 to 25 inclusive, assuming an ASCII string (more on that later).
Consider separating I/O from the algorithm
Right now, everything is done in main. Better practice is to separate things into functions. In particular, I'd recommend separating the input/output routines from the actual cipher.
Use better variable names
The variable name rotatorN
is good, but the name string4
is not. The first name explains something about what the variable means within the context of the code, but the latter is only confusing. A better name might be plaintext
.
Know what the C standard doesn't say
Your code makes the assumption that all lowercase letters are linearly organized in the range of 'a'
to 'z'
, but that's only true of ASCII. The C standard doesn't have such a requirement, and in fact, on a machine using EBCDIC encoding, your code would fail.
Use pointers rather than indexing for speed
Your program probably doesn't need a lot of speed, but you should know that using indexing is generally slower than using a pointer. In a loop such as the main one in your program, the increase in speed could be considerable for a very long string.
Don't use printf
where it isn't needed
Your code uses an odd loop to print the translated string, calling printf
with a format string of "%c"
. Rather than using that, use putchar()
or better still, replace the entire loop with puts()
.
Eliminate return 0
You don't need to explicitly provide a return 0;
at the end of main -- it's created implicitly by the compiler.
Consider an alternative approach
Rather than doing all of that calculation for each letter, you could use a simple lookup mechanism instead. Keep two contiguous alphabets in memory and use the plaintext char and rotate value to calculate an offset. Here's a complete program with most of these ideas incorporated, but note that it still won't work correctly with EBCDIC or a locale other than C:
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define ALPHABET_LEN 26
int encode(int ch, int rotate)
{
const char alphabet[2*ALPHABET_LEN] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz";
if (islower(ch)) {
return alphabet[ch-'a'+rotate];
} else {
return ch;
}
}
int main(int argc, char *argv[])
{
if (argc < 2) {
puts("Usage: caesar rotateN\n");
return 0;
}
int rotatorN = atoi(argv[1]) % ALPHABET_LEN;
for (int ch = getchar(); ch != EOF; ch = getchar()) {
putchar(encode(ch, rotatorN));
}
putchar('\n');
}
Here's an example of its use:
$ echo the quick brown fox jumped over the lazy dog | ./caesar -1
sgd pthbj aqnvm enw itlodc nudq sgd kyx cnf