Encryption/decryption program

Question

Here is a little encryptor I wrote, I'm just asking for people to tell me what is good practice and what's not. Also, I mean the code, not the actual encryption (because it's just a dumb thing).

#include "stdafx.h"
#include <iostream>
#include <stdexcept>

using namespace std;

int encrypter(int input);
int decrypter(int input);
bool whatToDoFunc();

int main()
{
    try
    {
        bool keepGoing = true;

        while (keepGoing)
        {
            bool success = whatToDoFunc(); char redo;
            cout << "Press 'y' to redo or anykey to exit" << endl;
            cin >> redo;
            if (success == true && (redo == 'y' || redo == 'Y'))
                keepGoing == true;
            else
                keepGoing = false;
        }
        }
    catch (runtime_error err)
    {
        cout << err.what() << endl;
        char exitCon;
        cout << "Enter anykey to exit" << endl;
        cin >> exitCon;
        return -1;
    }

    return 0;
}

bool whatToDoFunc()
{
    int input;
    char whatToDo;
    cout << "Welcome to the JeGo encrypter what do you want to do encrypt or     decrypy? Enter E or D\n" << endl;
    cin >> whatToDo;
    if (whatToDo == 'e' || whatToDo == 'E')
    {
        cout << "Enter the number you want to encrypt" << endl;
        cin >> input;
        int inputEncrypted = encrypter(input);
        cout << "Your number encrypted is " << inputEncrypted << endl;
    }

    else
    {
        if (whatToDo == 'd' || whatToDo == 'D')
        {
            cout << "Enter the already encrypted number" << endl;
            cin >> input;
            int numDecrypted = decrypter(input);
            cout << "Your number decrypted is " << numDecrypted << endl;
        }
        else
        {
            cerr << "Error" << endl;
            throw runtime_error("Your input was invalid");
        }
    }

return true;
}

int encrypter(int input)
{
    int inputEncrypted;
    inputEncrypted = ((((input * 2) + 7) + 5) + 8);
    if (inputEncrypted % 2 == 0)
        ++inputEncrypted;
    else
        inputEncrypted = inputEncrypted + (13 + 13) + (11 + 11);
    return inputEncrypted;
}

int decrypter(int inputEncrypted)
{
    int decryptedNum = 0;
    if (inputEncrypted % 2 == 1)
        --inputEncrypted;
    else
    {
        (11 - 11) - (13 - 13) - inputEncrypted;
    }
    decryptedNum = ((((inputEncrypted - 8) - 5) - 7) / 2);
    return decryptedNum;
}

Please tell me what I could do to make it better or just nicer design! :)

Answer 1

Bug?

It looks like there is a bug here:

else
{
    (11 - 11) - (13 - 13) - inputEncrypted;
}

After simplification, the expression becomes:

else
{
    -inputEncrypted;
}

... which does nothing, as the result is not assigned, the value of inputEncrypted is unchanged. If you wanted to negate the value of the variable, you would have to write like this:

else
{
    inputEncrypted = -inputEncrypted;
}

I don't know if this is your real intention (didn't look at your code close enough). But the original code does absolutely nothing, it's reduced to a statement whose result is not stored anywhere.

Use boolean expressions directly

This can be simplified:

if (success == true && (redo == 'y' || redo == 'Y'))
    keepGoing == true;
else
    keepGoing = false;

To this:

keepGoing = success && (redo == 'y' || redo == 'Y');

Notice how you don't need to write x == true, you can use boolean expressions directly.

Naming

A name like whatToDoFunc is obviously bad. Good function names are extremely important to understand how a program works.

Answer 2

encrypter

int encrypter(int input)
{
    return (input * 2) + 21;
}

Works the same as your function. To get to that I did the following:

• Folding compile time expression.
• Using ternaries.
• Noting that x * 2 + 20 is always even.
• Returning directly with no intermediate variable.

decrypter

decrypter is just the inverse of encrypter so:

int decrypter(int input)
{
    return (input - 21) / 2;
}

Answer 3

Naming

whatToDoFunc and whatToDo are bad names. Use appropriate naming.

Implementation

• Using using namespace std; is bad for ambiguity.
• Don't use std::endl, use '\n'.
• You can simplify some newbie expressions.

You have:

 if (success == true && (redo == 'y' || redo == 'Y'))
                keepGoing == true;
            else
                keepGoing = false;
        }

which can be simplified to:

keepGoing = success && (redo == 'y' || redo == 'Y');

---

The encrypter function can be simplified to:

int encrypter(int input)
{
    return (input * 2) + 21;
}

---

The decrypter function can be simplified to:

int decrypter(int inputEncrypted)
{
    int firstresult = inputEncrypted % 2 == 1 ? --inputEncrypted : ((11 - 11) - (13 - 13) - inputEncrypted);
    return ((firstresult - 20) / 2);
}

by using ternaries. Yes, you can do it in one line, but it's preferred to make it more readable.

---

• getchar() is useful in some cases.

  You have:

  cout << "Enter anykey to exit" << endl;
      cin >> exitCon;
  

  This is not right. Actually, the user needs to press ENTER, not any key. So, replace the cin >> exitCon; with getchar(); (avoid system() functions).

Maybe there is a bug in your program in the encrypt/decrypt functions.

Answer 4

• Signed integer overflow is undefined behavior. Use unsigned int and let it wrap around instead. Otherwise you risk a compiler "optimizing" your code by removing it by proving that it would cause an integer overflow.

• The operation * 2 is not reversible, because you may lose information. The highest bit that you are shifting away should be put to the first bit, making it a bit rotation operation.

  Example: If you were using unsigned char then 200 * 2 = 400 % 256 = 144 and 72 * 2 = 144 and if you want to decrypt a 144 you don't know if it should be 200 or 72. Same applies to integers, just with bigger numbers.

  The solution is to use bit rotation instead which is a bit awkward, but you can check it out here.

Answer 5

The usual recommendations apply:

• Test your code before posting it here. In particular, make sure that for every possible integer, encrypting it and decrypting it results in the same number.
• Enable all available compiler warnings and try to understand them. They prevent you from wasting other people's time. If there are warnings that you cannot understand, ask about them.