Caesar Cipher encryption/decryption

Question

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 <stdio.h>
#include <cs50.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
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;
}
}

• Snowbody: Can you post cs50.h?

Answer 1

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.

• Edward: 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.
• janos: Thanks @Edward, that's an excellent point. I added to my post.
• Jordan V: Thanks guys, really helpful. @ janos, is it usually recommended to then use 'A'/ 'a' instead of their numerical values (as long as it was consistent throughout the code)?
• janos: @JordanV absolutely! The readability benefit is crystal clear, and the code will compile to the exact same binary.

Answer 2

Get your types right

int main(int argc, string argv[])

I'm not aware of any C compiler that has a string type. I assume that this is a typedef or #define inside "cs50.h". It's a really bad idea, as it will make your code difficult to port to C++ if you ever choose to do so. In C, strings are not real types; arrays (or buffers) of char stand in for them. A quoted string is just a shortcut for creating a buffer; the string operation functions actually operate on buffers.

Separate user interface and business logic

Your main routine should handle input and output, but not any real logic. The actual logic of your program should be in a separate function. main should call that function.

Use simplest available function

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

putchar() would be a better choice for outputting a single char. But you really ought to be creating/modifying a buffer of char.

Answer 3

compilation Error

I encountered a compilation error for your code on the ideone c compiler.

prog.c:2:18: fatal error: cs50.h: No such file or directory #include

In pure C declaring a string using string method is not entertained it is more of a c++ method.