6
\$\begingroup\$

I am trying to implement a "Caeser cipher" to every lower case character in a string, by using a number to control the amount of character rotation. Here is an algorithm that I came up with. Can you please review and give me some feedback?

#include <stdio.h>
int main()
{
    char string4[80];
    int  rotatorN;

    printf("Enter String: ");
    gets(string4);
    printf("Enter Number: ");
    scanf("%i", &rotatorN);

    int n = 0, rotateSwap = 0;

    int i;
    for(i=0; string4[i] != '\0'; i++)
     {
          if(string4[i] >='a' && string4[i] <='z')
             {
                n = 'z' - string4[i];
                if( rotatorN > n )
                 {
                    rotateSwap = rotatorN - n - 1;
                    string4[i] = 'a';
                    string4[i] += rotateSwap;
                  }  
               else
                string4[i] += rotatorN;


               }
       }

      int j;
      for(j = 0; string4[j] !='\0'; j++)
        {
             printf("%c", string4[j]);
        }

        printf("\n");

        return 0;
}
\$\endgroup\$

5 Answers 5

5
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ I find it a bit awkward that you're defining a constant ALPHABET_LENGTH that cannot be changed without also changing a hardcoded string elsewhere. At the very least, there should be a comment about that; even better, you could do #define ALPHABET "abcdefghijklmnopqrstuvwxyz" right next to it (and const char alphabet[2*ALPHABET_LEN] = ALPHABET ALPHABET; later). Also, you seem to be spending considerable effort to avoid the % operator, but then use it anyway in main(). And if you're avoiding % and worrying about EBCDIC, why not go the last step and use strchr() instead of ch-'a'? \$\endgroup\$ Commented Mar 16, 2015 at 15:35
  • \$\begingroup\$ @IlmariKaronen: For the % operator, note that is only used exactly once, regardless of input string length. Also instead of using strchr(), I'd probably instead dyamically construct a translation table to use as a map, but that would be a more significant change to the original. \$\endgroup\$
    – Edward
    Commented Mar 16, 2015 at 16:00
8
\$\begingroup\$

You should never use gets() to read keyboard input, ever. Use fgets(), where you specify the max size of the buffer. That way, you are protecting yourself from somebody hacking your code.

You don't check input values. It is good practice to not assume a benevolent keyboard user, so, for instance, when a user enters rotatorN = -12128, you get all kinds of interesting behavior in your program.

Using scanf for input is hard to get right and therefore error-prone. Instead, use fgets for all input and then use sscanf to get the contents.

Check the calculated value for rotateSwap so that it falls within the expected range.

\$\endgroup\$
6
\$\begingroup\$
  • Did they already mentioned gets? If not, I'd reiterate: Don't use gets. It makes your code vulnerable to buffer overflow; besides, it is deprecated. If your compiler didn't warn you, consider upgrading.

  • Don't be shy of functions. Your main does too much: it deals with input, encryption and output all at once. Decompose it into single responsibility units.

  • string4 is a very unnerving name. The reviewer immediately starts worrying: where is string3? did I miss it?.

    I would also consider renaming rotatorN and rotateSwap (the former is an encryption key - why not call it so?; the latter is a verb - a good name for a function perhaps, but not for a variable).

\$\endgroup\$
2
\$\begingroup\$

A few minor things not already mentioned:

  • Try to maintain consistent indentation. It can really help for maintaining readability.

  • Add a void parameter for main() so that the C compiler will know that it doesn't take command line arguments (or any arguments for other functions in general).

  • The first two outputs can use puts() instead. It's really only necessary to use printf() for formatted output, such as the one in your last for loop.

I think it bears repeating: always use fgets() instead of gets(). The latter is only still around to prevent breaking of existing code, and even Unix's man page explicitly warns about its use.

\$\endgroup\$
1
\$\begingroup\$

Your code looks OK. But, just consider the case when rotatorN is greater than 26. Say, for example, rotatorN is 100 and string4 is "a".

Then for the first iteration:

n = 'z' - 'a' = 25
rotateSwap = 100 - 25 - 1 = 74
string4[0] = string4[0] + 74 = 97 + 74 = 171,

171 is a part of the extended ASCII set and is not exactly what you expected.

In order avoid this issue, you have take the modulus of rotatorN. For instance, just after reading rotatorN, perform rotatorN = rotatorN % 26.

\$\endgroup\$
0

Not the answer you're looking for? Browse other questions tagged or ask your own question.