HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Encryption/decryption program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
encryptionprogramdecryption

Problem

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 
#include 

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 > redo;
            if (success == true && (redo == 'y' || redo == 'Y'))
                keepGoing == true;
            else
                keepGoing = false;
        }
        }
    catch (runtime_error err)
    {
        cout > exitCon;
        return -1;
    }

    return 0;
}

bool whatToDoFunc()
{
    int input;
    char whatToDo;
    cout > whatToDo;
    if (whatToDo == 'e' || whatToDo == 'E')
    {
        cout > input;
        int inputEncrypted = encrypter(input);
        cout > 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! :)

Solution

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.

Code Snippets

else
{
    (11 - 11) - (13 - 13) - inputEncrypted;
}
else
{
    -inputEncrypted;
}
else
{
    inputEncrypted = -inputEncrypted;
}
if (success == true && (redo == 'y' || redo == 'Y'))
    keepGoing == true;
else
    keepGoing = false;
keepGoing = success && (redo == 'y' || redo == 'Y');

Context

StackExchange Code Review Q#113091, answer score: 15

Revisions (0)

No revisions yet.