Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
2  
Note: the line (11 - 11) - (13 - 13) - inputEncrypted; does absolutely nothing. – immibis 12 hours ago
    
@immibis not right. That instruction can bring inputEncrypted to a negative number. – Pr0kram3r 6 hours ago
    
@Pr0kram3r Except that it is not saved... – Sumurai8 5 hours ago
    
That's not what @OP was intended to do. – Pr0kram3r 5 hours ago

4 Answers 4

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.

share|improve this answer
    
If I pass 16 as parameter to inputEncrypted, which is positive, that instruction brings the number to a negative number. 11 - 11 is 0 and 13 - 13 is 0. 0 - 0 - 16= -16. I don't know what you do mean in your answer. – Pr0kram3r 6 hours ago
    
@Pr0kram3r how should this happen without assigning it to any variable ? – Heslacher 6 hours ago
    
@Heslacher I didn't note that. So, it is a bug. – Pr0kram3r 5 hours ago
1  
@Pr0kram3r I didn't think the pointlessness of that statement needed any explanation. I clarified it now, just in case. – janos 4 hours ago

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;
}
share|improve this answer
    
Ok I changed it – Magirldooger 19 hours ago

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.

share|improve this answer
  • 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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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