That looks a lot like homework and you didn't seem to make much effort on finding the issues, but here we go.. There are several flaws on the code:
1) Ignoring return values:
Some of your functions, getUserChoice
and getShift
, are supposed to return a value but those values are completely ignored in the code. You are not assigning it to any local variables and they are not modifying anything outside their scope, so whatever value is being returned is getting lost.
The code:
if(number = 1){
getShift();
}
Should be:
if(number == 1){
shift = getShift();
}
Which brings me to another big mistake, reason of many headaches on C when it happens unintentionally:
2) Mistakenly writing the assignment operator '=' where a comparison operator '==' is required:
The assignment operator returns a value: the left operand value after assignment. So the expression if(number = 1)
is valid and will evaluate to true every time, since the returning value of the assignment is always 1. if(number == 1)
however, will only evaluate to true when number
is equal to 1. I find it useful to make comparisons "Yoda-Style", like if(1 == number)
to avoid those kind of mistakes (that can easily happen on a typo). They are hard to track, since the code is valid, but the behavior is not the expected. Using the "Yoda-Style" comparison, you would get a compiler error if you mistyped if(1 = number)
, since that's an invalid assignment.
3) Not properly filtering the user input:
On the function getUserChoice
you have the following conditional statement to check if the input is valid (when invalid it defaults to the "Quit" option):
if(decision == 0 || decision >= 4)
{
return 0;
}
However, since decision is a signed int
, negative values are valid. You don't account for those. The following addresses that:
if(decision <= 0 || decision >= 4)
{
return 0;
}
4) getShift
doesn't return the shift value:
You used scanf
on the getShift
function to get a shift value, but returned 0. You should return the key
value instead.
5) Unused variable at getString
+ Wrong size argument on fgets
:
You declared an array of 0's on getString
which serves no purpose on it. Also, from fgets
man page:
fgets() reads in at most one less than size characters from stream and
stores them into the buffer pointed to by s. Reading stops after an
EOF or a newline. If a newline is read, it is stored into the buffer.
A terminating null byte ('\0') is stored after the last character in
the buffer.
So you don't really need to subtract 1 from SIZE, since it will already account for the null character.
6) Not clearing the stdin
before calling fgets
:
After the call to scanf("%d",...);
, you will have a hanging newline character in your stdin
. That's because the format string parsed to scanf
there only instructs it to read any number of decimal digits, not including the newline that follows those.
That hanging newline would cause your fgets
to quit before getting any user input. You need to clear the stdin
by reading the hanging characters from a previous scanf
call. In the example, this is being done by this loop:
int c;
while((c = getchar()) != '\n' && c != EOF){
//Clearing Stdin
}
An alternative is to use:
scanf("%*[^\n]"); scanf("%*c"); //Consume all characters up to \n (if present) then consume exactly one char ('\n' itself)
7) Not using a loop in main to prompt the user repeatedly:
You only prompt the user once, do what is asked and quit, instead of asking the user repeatedly for options until he asks to quit.
8) Unnecessary return
statements on the end of void
returning functions:
void
return functions will return when they reach their end, so there is no need to add a return
statement unless you are returning before the end of the function body.
The code below is edited to fix the mentioned issues:
#include <stdio.h>
#include <string.h>
#define SIZE 500
int getUserChoice()
{
int decision = 0;
printf("-------------------------------\n");
printf("| 1) Change Shift (default 3) |\n");
printf("| 2) Encrypt a message |\n");
printf("| 3) Decrypt a message |\n");
printf("| 4) Quit |\n");
printf("-------------------------------\n");
printf("Option: ");
scanf("%d", &decision);
int c;
while((c = getchar()) != '\n' && c != EOF){
//Clearing Stdin
}
if(decision <= 0 || decision >= 4)
{
return 0;
}
return decision;
}
int getShift()
{
int key = 3;
printf("Enter new shift value: ");
scanf("%d", &key);
int c;
while((c = getchar()) != '\n' && c != EOF){
//Clearing Stdin
}
return key;
}
void getString(char *buf)
{
fgets(buf, SIZE, stdin);
return;
}
void encrypt(char *buf, int shift)
{
int i = 0;
for(i = 0; i <= (strlen(buf)-1);++i)
{
buf[i] = buf[i] + shift;
}
return;
}
void decrypt(char *buf, int shift)
{
int i = 0;
for(i = 0; i <= (strlen(buf)-1); ++i)
{
buf[i] = buf[i] - shift;
}
return;
}
int main()
{
char userStr[SIZE];
int shift = 3;
while(1){
int number = getUserChoice();
if(number == 1)
{
shift = getShift();
}
if (number == 2)
{
getString(userStr);
printf("Message: %s\n", userStr);
encrypt(userStr, shift);
printf("Shifted Message: %s\n", userStr);
}
if (number == 3)
{
getString(userStr);
printf("Shifted Message: %s\n", userStr);
decrypt(userStr, shift);
printf("Message: %s\n", userStr);
}
if (number == 0){
break;
}
}
return 0;
}