HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Caesar Cipher encryption/decryption

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
encryptioncaesardecryptioncipher

Problem

This program takes a command line argument of how many times you would like to encrypt plain text. After you compile the program, input a message you would like to have coded. I'm new to C and curious about how to make this program more efficient.

/*
*caesar.c
*
*
*
*Encrypts user supplied messages according to a user supplied   encryption key 
*
*/

#include 
#include 
#include 
#include 
#include 

int main(int argc, string argv[])
{

  if (argc != '\0' && argc == 2 )   
  { 

    //converting string input to integers
    int f= atoi(argv[1]);

    //getting string command from user
    string a = GetString();

    for(int i = 0, j = strlen(a); i < j; i++)
    {

        if ( isalpha (a[i]))
        {
            if (isupper(a[i]))
            {
                //converting capitalized chars
                char acap = (a[i] - 65);
                int ccap = (acap+f)%26;
                //final capitalized chars loop
                char ecap = ccap + 65;
                printf("%c", ecap);
            }

            else if (islower(a[i]))
            {
                //converting small chars
                char asma = (a[i] - 97);
                int csma = (asma+f)%26;
                //final small chars loop
                char esma = csma + 97;
                printf("%c", esma);
            }

       }
       else
       {
           printf("%c", a[i]);
       }

    }            printf("%c", esma);
    return 0;
}   
else   
{
    return 1;
}
}

Solution

This is unnecessarily complicated:

if (argc != '\0' && argc == 2 )


This is exactly the same thing but simpler:

if (argc == 2)


Instead of the hard-coded ASCII code 65 of "A", you could use 'A' to make the code easier to read. So instead of this:

//converting capitalized chars
char acap = (a[i] - 65);
int ccap = (acap+f)%26;
//final capitalized chars loop
char ecap = ccap + 65;
printf("%c", ecap);


You can write like this:

//converting capitalized chars
char acap = (a[i] - 'A');
int ccap = (acap + f) % 26;
//final capitalized chars loop
char ecap = ccap + 'A';
printf("%c", ecap);


I also added spaces around operators in (acap+f)%26 to make it more readable, which is a good practice.

The same goes for the number 97, it's better to use 'a' instead.

The code for handling upper case and lower case letters is practically the same, except for the constant 'A' and 'a'.
Avoid such code duplication, repetition and copy-paste coding as much as possible.
Extract the common logic to a helper function,
and reuse it by passing 'A' and 'a' as parameter.

Use more descriptive variable names.
f, acap, ccap, ecap are really not intuitive,
and your code could become much more readable if you gave these better names.

@Edward made an excellent remark in comments that I'm just going to quote verbatim:


It's worth noting that both original and suggested changes assume a
character encoding with a contiguous encoding for the alphabet. This
is not actually guaranteed by the C standard. For example, EBCDIC
systems are admittedly rare but not yet extinct.

Code Snippets

if (argc != '\0' && argc == 2 )
if (argc == 2)
//converting capitalized chars
char acap = (a[i] - 65);
int ccap = (acap+f)%26;
//final capitalized chars loop
char ecap = ccap + 65;
printf("%c", ecap);
//converting capitalized chars
char acap = (a[i] - 'A');
int ccap = (acap + f) % 26;
//final capitalized chars loop
char ecap = ccap + 'A';
printf("%c", ecap);

Context

StackExchange Code Review Q#88578, answer score: 5

Revisions (0)

No revisions yet.