9
\$\begingroup\$

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! :)

\$\endgroup\$
  • 4
    \$\begingroup\$ Note: the line (11 - 11) - (13 - 13) - inputEncrypted; does absolutely nothing. \$\endgroup\$ – user253751 Dec 7 '15 at 4:22
  • \$\begingroup\$ @immibis not right. That instruction can bring inputEncrypted to a negative number. \$\endgroup\$ – user91060 Dec 7 '15 at 10:39
  • 3
    \$\begingroup\$ @Pr0kram3r Except that it is not saved... \$\endgroup\$ – Sumurai8 Dec 7 '15 at 11:08
  • \$\begingroup\$ That's not what @OP was intended to do. \$\endgroup\$ – user91060 Dec 7 '15 at 11:11
15
\$\begingroup\$

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.

\$\endgroup\$
14
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Ok I changed it \$\endgroup\$ – Magirldooger Dec 6 '15 at 21:51
  • \$\begingroup\$ I frequently see parenthesis being used unnecessarily, like you did here return (input * 2) + 21;. Is it supposed to be like this to avoid confusion or you made it this way because you/the OP/someone may not know that * gets evaluated before +? This kind of rule seems pretty straightforward and I think it works the same for every language. \$\endgroup\$ – Tiago Marinho Dec 7 '15 at 17:26
  • \$\begingroup\$ @TiagoMarinho In simple expressions it is the same to use or not parenthesis. \$\endgroup\$ – Caridorc Dec 7 '15 at 17:44
  • \$\begingroup\$ @TiagoMarinho, yeah I think this same thing. But some people really like not having to memorize the order of operations, e.g. a || b && c. Personally I think they should get over it or find a different language that fits their preferences (Lisp). \$\endgroup\$ – Paul Draper Dec 8 '15 at 20:12
6
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$
  • 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.

\$\endgroup\$
0
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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