12
\$\begingroup\$

I'm trying to improve my C++ Enigma Machine, either by shortening the code or finding more efficient ways to complete certain things. Be aware I have very basic C++ vocabulary. Are there any major flaws in my code? Also, would anyone happen to know how I could do decryption properly? I asked on Stack Overflow and it wasn't received too well and one of the comments said code that's pretty much done but still needs some final touches belongs on this site. I would really appreciate some help.

Note: I used the 1998 version of C++ instead of C++ 11 or C++ 14 because I needed access to the auto keyword to store the raw data of a function instead of the output.

//Enigma Machine
#include <iostream>
#include <string>
#include <conio.h>
#include <algorithm>
#include <vector>
#include <iterator>
#include <array>
//Namespace
using namespace std;
//Global variables
string option;
string choiceforencr;
string chosen_roter_one_s;
string chosen_roter_two_s;
string chosen_roter_three_s;
string currentLetter;
string encrypt_decrypt_output = "";
int chosen_roter_one;
int chosen_roter_two;
int chosen_roter_three;
int rotate_position_one;
int rotate_position_two;
int rotate_position_three;
int error = 0;
bool encrypt_Toggle = true;
int encr_decry_length;
//Function declarations
int enc_Decry_f_string();
//Roters
int roter_1(){
        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};
        string myArray_one_decrypt[26] = {"z","y","x","w","v","u","t","s","r","q","p","o","n","m","l","k","j","i", "h","g","f","e","d","c","b","a"};
    // rotation to the right by x amount
    if (encrypt_Toggle == true){
        rotate(reverse_iterator<string*>(end(myArray_one)),
                reverse_iterator<string*>(end(myArray_one)) + rotate_position_one,
                reverse_iterator<string*>(myArray_one));
    }
    else if (encrypt_Toggle == false){
        rotate(reverse_iterator<string*>(end(myArray_one_decrypt)),
                reverse_iterator<string*>(end(myArray_one_decrypt)) + (rotate_position_one + 26),
                reverse_iterator<string*>(myArray_one_decrypt));
    }
    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
    else if(currentLetter == "e"){currentLetter = myArray_one[4];}
    else if(currentLetter == "f"){currentLetter = myArray_one[5];}
    else if(currentLetter == "g"){currentLetter = myArray_one[6];}
    else if(currentLetter == "h"){currentLetter = myArray_one[7];}
    else if(currentLetter == "i"){currentLetter = myArray_one[8];}
    else if(currentLetter == "j"){currentLetter = myArray_one[9];}
    else if(currentLetter == "k"){currentLetter = myArray_one[10];}
    else if(currentLetter == "l"){currentLetter = myArray_one[11];}
    else if(currentLetter == "m"){currentLetter = myArray_one[12];}
    else if(currentLetter == "n"){currentLetter = myArray_one[13];}
    else if(currentLetter == "o"){currentLetter = myArray_one[14];}
    else if(currentLetter == "p"){currentLetter = myArray_one[15];}
    else if(currentLetter == "q"){currentLetter = myArray_one[16];}
    else if(currentLetter == "r"){currentLetter = myArray_one[17];}
    else if(currentLetter == "s"){currentLetter = myArray_one[18];}
    else if(currentLetter == "t"){currentLetter = myArray_one[19];}
    else if(currentLetter == "u"){currentLetter = myArray_one[20];}
    else if(currentLetter == "v"){currentLetter = myArray_one[21];}
    else if(currentLetter == "w"){currentLetter = myArray_one[22];}
    else if(currentLetter == "x"){currentLetter = myArray_one[23];}
    else if(currentLetter == "y"){currentLetter = myArray_one[24];}
    else if(currentLetter == "z"){currentLetter = myArray_one[25];}
    else{currentLetter = currentLetter;}
rotate_position_one += 1;
}
int roter_2(){
        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};
        string myArray_one_decrypt[26] = {"z","y","x","w","v","u","t","s","r","q","p","o","n","m","l","k","j","i", "h","g","f","e","d","c","b","a"};
    // rotation to the right by x amount
    if (encrypt_Toggle == true){
        rotate(reverse_iterator<string*>(end(myArray_one)),
                reverse_iterator<string*>(end(myArray_one)) + rotate_position_one,
                reverse_iterator<string*>(myArray_one));
    }
    else if (encrypt_Toggle == false){
        rotate(reverse_iterator<string*>(end(myArray_one_decrypt)),
                reverse_iterator<string*>(end(myArray_one_decrypt)) + (rotate_position_one + 26) ,
                reverse_iterator<string*>(myArray_one_decrypt));
    }
    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
    else if(currentLetter == "e"){currentLetter = myArray_one[4];}
    else if(currentLetter == "f"){currentLetter = myArray_one[5];}
    else if(currentLetter == "g"){currentLetter = myArray_one[6];}
    else if(currentLetter == "h"){currentLetter = myArray_one[7];}
    else if(currentLetter == "i"){currentLetter = myArray_one[8];}
    else if(currentLetter == "j"){currentLetter = myArray_one[9];}
    else if(currentLetter == "k"){currentLetter = myArray_one[10];}
    else if(currentLetter == "l"){currentLetter = myArray_one[11];}
    else if(currentLetter == "m"){currentLetter = myArray_one[12];}
    else if(currentLetter == "n"){currentLetter = myArray_one[13];}
    else if(currentLetter == "o"){currentLetter = myArray_one[14];}
    else if(currentLetter == "p"){currentLetter = myArray_one[15];}
    else if(currentLetter == "q"){currentLetter = myArray_one[16];}
    else if(currentLetter == "r"){currentLetter = myArray_one[17];}
    else if(currentLetter == "s"){currentLetter = myArray_one[18];}
    else if(currentLetter == "t"){currentLetter = myArray_one[19];}
    else if(currentLetter == "u"){currentLetter = myArray_one[20];}
    else if(currentLetter == "v"){currentLetter = myArray_one[21];}
    else if(currentLetter == "w"){currentLetter = myArray_one[22];}
    else if(currentLetter == "x"){currentLetter = myArray_one[23];}
    else if(currentLetter == "y"){currentLetter = myArray_one[24];}
    else if(currentLetter == "z"){currentLetter = myArray_one[25];}
    else{currentLetter = currentLetter;}
rotate_position_one += 1;
}
int roter_3(){
        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};
        string myArray_one_decrypt[26] = {"z","y","x","w","v","u","t","s","r","q","p","o","n","m","l","k","j","i", "h","g","f","e","d","c","b","a"};
    // rotation to the right by x amount
    if (encrypt_Toggle == true){
        rotate(reverse_iterator<string*>(end(myArray_one)),
                reverse_iterator<string*>(end(myArray_one)) + rotate_position_one,
                reverse_iterator<string*>(myArray_one));
    }
    else if (encrypt_Toggle == false){
        rotate(reverse_iterator<string*>(end(myArray_one_decrypt)),
                reverse_iterator<string*>(end(myArray_one_decrypt)) + (rotate_position_one + 26),
                reverse_iterator<string*>(myArray_one_decrypt));
    }
    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
    else if(currentLetter == "e"){currentLetter = myArray_one[4];}
    else if(currentLetter == "f"){currentLetter = myArray_one[5];}
    else if(currentLetter == "g"){currentLetter = myArray_one[6];}
    else if(currentLetter == "h"){currentLetter = myArray_one[7];}
    else if(currentLetter == "i"){currentLetter = myArray_one[8];}
    else if(currentLetter == "j"){currentLetter = myArray_one[9];}
    else if(currentLetter == "k"){currentLetter = myArray_one[10];}
    else if(currentLetter == "l"){currentLetter = myArray_one[11];}
    else if(currentLetter == "m"){currentLetter = myArray_one[12];}
    else if(currentLetter == "n"){currentLetter = myArray_one[13];}
    else if(currentLetter == "o"){currentLetter = myArray_one[14];}
    else if(currentLetter == "p"){currentLetter = myArray_one[15];}
    else if(currentLetter == "q"){currentLetter = myArray_one[16];}
    else if(currentLetter == "r"){currentLetter = myArray_one[17];}
    else if(currentLetter == "s"){currentLetter = myArray_one[18];}
    else if(currentLetter == "t"){currentLetter = myArray_one[19];}
    else if(currentLetter == "u"){currentLetter = myArray_one[20];}
    else if(currentLetter == "v"){currentLetter = myArray_one[21];}
    else if(currentLetter == "w"){currentLetter = myArray_one[22];}
    else if(currentLetter == "x"){currentLetter = myArray_one[23];}
    else if(currentLetter == "y"){currentLetter = myArray_one[24];}
    else if(currentLetter == "z"){currentLetter = myArray_one[25];}
    else{currentLetter = currentLetter;}
rotate_position_one += 1;
}
int roter_4(){
        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};
        string myArray_one_decrypt[26] = {"z","y","x","w","v","u","t","s","r","q","p","o","n","m","l","k","j","i", "h","g","f","e","d","c","b","a"};
    // rotation to the right by x amount
    if (encrypt_Toggle == true){
        rotate(reverse_iterator<string*>(end(myArray_one)),
                reverse_iterator<string*>(end(myArray_one)) + rotate_position_one,
                reverse_iterator<string*>(myArray_one));
    }
    else if (encrypt_Toggle == false){
        rotate(reverse_iterator<string*>(end(myArray_one_decrypt)),
                reverse_iterator<string*>(end(myArray_one_decrypt)) + (rotate_position_one + 26),
                reverse_iterator<string*>(myArray_one_decrypt));
    }
    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
    else if(currentLetter == "e"){currentLetter = myArray_one[4];}
    else if(currentLetter == "f"){currentLetter = myArray_one[5];}
    else if(currentLetter == "g"){currentLetter = myArray_one[6];}
    else if(currentLetter == "h"){currentLetter = myArray_one[7];}
    else if(currentLetter == "i"){currentLetter = myArray_one[8];}
    else if(currentLetter == "j"){currentLetter = myArray_one[9];}
    else if(currentLetter == "k"){currentLetter = myArray_one[10];}
    else if(currentLetter == "l"){currentLetter = myArray_one[11];}
    else if(currentLetter == "m"){currentLetter = myArray_one[12];}
    else if(currentLetter == "n"){currentLetter = myArray_one[13];}
    else if(currentLetter == "o"){currentLetter = myArray_one[14];}
    else if(currentLetter == "p"){currentLetter = myArray_one[15];}
    else if(currentLetter == "q"){currentLetter = myArray_one[16];}
    else if(currentLetter == "r"){currentLetter = myArray_one[17];}
    else if(currentLetter == "s"){currentLetter = myArray_one[18];}
    else if(currentLetter == "t"){currentLetter = myArray_one[19];}
    else if(currentLetter == "u"){currentLetter = myArray_one[20];}
    else if(currentLetter == "v"){currentLetter = myArray_one[21];}
    else if(currentLetter == "w"){currentLetter = myArray_one[22];}
    else if(currentLetter == "x"){currentLetter = myArray_one[23];}
    else if(currentLetter == "y"){currentLetter = myArray_one[24];}
    else if(currentLetter == "z"){currentLetter = myArray_one[25];}
    else{currentLetter = currentLetter;}
rotate_position_one += 1;
}
int roter_5(){
        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};
        string myArray_one_decrypt[26] = {"z","y","x","w","v","u","t","s","r","q","p","o","n","m","l","k","j","i", "h","g","f","e","d","c","b","a"};
    // rotation to the right by x amount
    if (encrypt_Toggle == true){
        rotate(reverse_iterator<string*>(end(myArray_one)),
                reverse_iterator<string*>(end(myArray_one)) + rotate_position_one,
                reverse_iterator<string*>(myArray_one));
    }
    else if (encrypt_Toggle == false){
        rotate(reverse_iterator<string*>(end(myArray_one_decrypt)),
                reverse_iterator<string*>(end(myArray_one_decrypt)) + (rotate_position_one + 26),
                reverse_iterator<string*>(myArray_one_decrypt));
    }
    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
    else if(currentLetter == "e"){currentLetter = myArray_one[4];}
    else if(currentLetter == "f"){currentLetter = myArray_one[5];}
    else if(currentLetter == "g"){currentLetter = myArray_one[6];}
    else if(currentLetter == "h"){currentLetter = myArray_one[7];}
    else if(currentLetter == "i"){currentLetter = myArray_one[8];}
    else if(currentLetter == "j"){currentLetter = myArray_one[9];}
    else if(currentLetter == "k"){currentLetter = myArray_one[10];}
    else if(currentLetter == "l"){currentLetter = myArray_one[11];}
    else if(currentLetter == "m"){currentLetter = myArray_one[12];}
    else if(currentLetter == "n"){currentLetter = myArray_one[13];}
    else if(currentLetter == "o"){currentLetter = myArray_one[14];}
    else if(currentLetter == "p"){currentLetter = myArray_one[15];}
    else if(currentLetter == "q"){currentLetter = myArray_one[16];}
    else if(currentLetter == "r"){currentLetter = myArray_one[17];}
    else if(currentLetter == "s"){currentLetter = myArray_one[18];}
    else if(currentLetter == "t"){currentLetter = myArray_one[19];}
    else if(currentLetter == "u"){currentLetter = myArray_one[20];}
    else if(currentLetter == "v"){currentLetter = myArray_one[21];}
    else if(currentLetter == "w"){currentLetter = myArray_one[22];}
    else if(currentLetter == "x"){currentLetter = myArray_one[23];}
    else if(currentLetter == "y"){currentLetter = myArray_one[24];}
    else if(currentLetter == "z"){currentLetter = myArray_one[25];}
    else{currentLetter = currentLetter;}
rotate_position_one += 1;
}
    auto final_Roter_one = roter_1;
    auto final_Roter_two = roter_2;
    auto final_Roter_three = roter_3;
//Other functions
void encrypt_enigma(){
    //Defaults
    auto final_Roter_one = roter_1;
    auto final_Roter_two = roter_2;
    auto final_Roter_three = roter_3;
    //chosen roter one
    if(chosen_roter_one == 1){
        final_Roter_one = roter_1;

    }
    else if(chosen_roter_one == 2){
        final_Roter_one = roter_2;

    }
    else if(chosen_roter_one == 3){
        final_Roter_one = roter_3;

    }
    else if(chosen_roter_one == 4){
        final_Roter_one = roter_4;

    }
    else{
        final_Roter_one = roter_5;

    }
    //Chosen roter two
    if(chosen_roter_two == 1){
        final_Roter_two = roter_1;

    }
    else if(chosen_roter_two == 2){
        final_Roter_two = roter_2;

    }
    else if(chosen_roter_two == 3){
        final_Roter_two = roter_3;

    }
    else if(chosen_roter_two == 4){
        final_Roter_two = roter_4;

    }
    else{
        final_Roter_two = roter_5;

    }
    //chosen roter three
    if(chosen_roter_three == 1){
        final_Roter_three = roter_1;

    }
    else if(chosen_roter_three == 2){
        final_Roter_three = roter_2;

    }
    else if(chosen_roter_three == 3){
        final_Roter_three = roter_3;

    }
    else if(chosen_roter_three == 4){
        final_Roter_three = roter_4;

    }
    else{
        final_Roter_three = roter_5;

    }
    enc_Decry_f_string();
}
class EncryDecry
{

    public:
        void updateStrings(string sentence){
            currentLetter = sentence[0];

        int stringLength = sentence.length();
        string wordLetters[stringLength];
        int initializer = 0;

        do{
        wordLetters[initializer] = currentLetter;
        currentLetter = sentence[initializer];
        initializer++;
        //Do it
        final_Roter_one();
        final_Roter_two();
        final_Roter_three();
        encrypt_decrypt_output += currentLetter;

        }while(initializer != stringLength);

        }
};
//Change the string
int enc_Decry_f_string(){
    EncryDecry machine;
    encr_decry_length = choiceforencr.length();
    machine.updateStrings(choiceforencr);
    printf("No segmentation fault");
}
void r_option(){
    cout << "Sorry that is not a valid input please try again(e/d)" << endl;
    cin >> choiceforencr;
}

void chose_a_roter(int stateof){
        int problem = 0;
        string message;

        if (stateof == 0){message = "What roters(1, 2, 3, 4, 5) would you like to use (You may only use 3):\n";}
        else{message = "Sorry your one of your last choices was not valid please try again\nWhat roters(1, 2, 3, 4, 5) would you like to use (You may only use 3):\n";}
        cout << message;
        cin >> chosen_roter_one_s;
        cin >> chosen_roter_two_s;
        cin >> chosen_roter_three_s;
        cout << "please enter your roter position(1-26)" << endl;
        do{
        if (problem >= 1){
            cout << "Sorry one of your last entrys was not valid please try again\nPlease enter your roter position" << endl;
        }
        else{
        cin >> rotate_position_one;
        cin >> rotate_position_two;
        cin >> rotate_position_three;

        }
        while(rotate_position_one >= 27 || rotate_position_two >= 27 || rotate_position_three >= 27);

        if (stateof == 1)
            if(chosen_roter_one == chosen_roter_two || chosen_roter_two == chosen_roter_three || chosen_roter_three == chosen_roter_one || chosen_roter_one > 5 || chosen_roter_two > 5 || chosen_roter_three > 5){
                error = 1;
            }
            else{
                error = 0;
            }
        }while(error == 1);
}
void convert(){
//positions for roter one

    do{
        int gone_trough = 0;
        int reason = 0;
    //if there was a problem...
        if (error == 1 && gone_trough > 0){
            chose_a_roter(1);
        }

    //Chosen roter one
        if (chosen_roter_one_s == "1" || chosen_roter_one_s == "one"){chosen_roter_one = 1;}
        else if (chosen_roter_one_s == "2" || chosen_roter_one_s == "two"){chosen_roter_one = 2;}
        else if (chosen_roter_one_s == "3" || chosen_roter_one_s == "three"){chosen_roter_one = 3;}
        else if (chosen_roter_one_s == "4" || chosen_roter_one_s == "four"){chosen_roter_one = 4;}
        else if (chosen_roter_one_s == "5" || chosen_roter_one_s == "five"){chosen_roter_one = 5;}
        else{
            error = 1;
        }
    //chosen Roter two
        if (chosen_roter_two_s == "1" || chosen_roter_two_s == "one"){chosen_roter_two = 1;}
        else if (chosen_roter_two_s == "2" || chosen_roter_two_s == "two"){chosen_roter_two = 2;}
        else if (chosen_roter_two_s == "3" || chosen_roter_two_s == "three"){chosen_roter_two = 3;}
        else if (chosen_roter_two_s == "4" || chosen_roter_two_s == "four"){chosen_roter_two = 4;}
        else if (chosen_roter_two_s == "5" || chosen_roter_two_s == "five"){chosen_roter_two = 5;}
        else{
            error = 1;
        }
    //Chosen Roter three
        if (chosen_roter_three_s == "1" || chosen_roter_three_s == "one"){chosen_roter_three = 1;}
        else if (chosen_roter_three_s == "2" || chosen_roter_three_s == "two"){chosen_roter_three = 2;}
        else if (chosen_roter_three_s == "3" || chosen_roter_three_s == "three"){chosen_roter_three = 3;}
        else if (chosen_roter_three_s == "4" || chosen_roter_three_s == "four"){chosen_roter_three = 4;}
        else if (chosen_roter_three_s == "5" || chosen_roter_three_s == "five"){chosen_roter_three = 5;}
        else{
            error = 1;
        }
        gone_trough++;
    }while(error == 1 || chosen_roter_one == chosen_roter_two || chosen_roter_two == chosen_roter_three || chosen_roter_three == chosen_roter_one);

}



int
main ()
{
bool repeat = true;
while(repeat){
    option = "";
    choiceforencr = "";
    chosen_roter_one_s = "";
        chosen_roter_two_s = "";
        chosen_roter_three_s = "";
            currentLetter = "";
            encrypt_decrypt_output = "";
                chosen_roter_one = 0;
                chosen_roter_two = 0;
            chosen_roter_three = 0;
            rotate_position_one = 0;
        rotate_position_two = 0;
        rotate_position_three = 0;
    error = 0;
    encr_decry_length = 0;
        error = 0;
        encrypt_Toggle = NULL;
            encrypt_decrypt_output = "";
  cout << "Welcome to the Enigma console. Are you encrypting or decrypting(e/d) or to end the program press Q" << endl;

  cin >> option;

  if (option == "e" || option == "encrypting")
    {
      encrypt_Toggle = true;
      cout << "You are encrypting what would you like to encrypte:\n";
      cin >> choiceforencr;
      chose_a_roter(0);
      convert ();
      encrypt_enigma();
      cout << "Your encrypted text is <<" << encrypt_decrypt_output << ">>" << endl;
    }
  else if (option == "d" || option == "decrypting")
    {
      encrypt_Toggle = false;
      cout << "You are decrypting what would you like to decrypte:\n";
      cin >> choiceforencr;
      chose_a_roter(0);
      convert();
      encrypt_enigma();
      cout << "Your decrypted text is <<" << encrypt_decrypt_output << ">>" << endl;
    }
  else if (option == "q"){
    return 0;
  }

  else
    {
      do{
        r_option ();
    }while (option != "e" || option != "d" || option != "decrypting" || option != "encrypting" || option != "q");
    }
}
}
\$\endgroup\$
  • 4
    \$\begingroup\$ auto in C++98 means automatic storage duration object. This was also the default. In C++98 if you did not specify a type then int was assumed. So in C++98 auto final_Roter_one = roter_1; Is the same as auto int final_Roter_one = roter_1; which is the same as int final_Roter_one = roter_1;. The auto keyword was redundant. \$\endgroup\$ – Martin York Apr 26 '18 at 22:08
  • 2
    \$\begingroup\$ @MartinYork, it is not decltype, but template type deduction. They are very different. \$\endgroup\$ – Incomputable Apr 27 '18 at 5:31
  • 2
    \$\begingroup\$ Namespaces were added in 1990. Though may be compilers didn’t support them well. \$\endgroup\$ – Incomputable Apr 27 '18 at 6:09
  • 1
    \$\begingroup\$ @MartinYork C++98 had namespaces :). You are probably thinking of the original C with classes or variations thereof. C++98 compilers are really easy to find: Take gcc or clang for example. \$\endgroup\$ – Rakete1111 Apr 27 '18 at 16:50
  • 3
    \$\begingroup\$ The English word is "rotor", not "roter" (which isn't a real word). en.wikipedia.org/wiki/Enigma_rotor_details. \$\endgroup\$ – Peter Cordes Apr 27 '18 at 17:33
23
\$\begingroup\$

Avoid Globals

Right now, you tell each rotor whether to encrypt or decrypt by setting the global variable encrypt_Toggle to false or true.

It would be much better to instead pass that value as a parameter to the rotor functions.

Avoid Boolean parameters

When you do that, though, I'd advise against using a Boolean. It would be much better to define an enumeration something like:

enum { encrypt, decrypt };

...so when you call the function, it's something like:

roter_1(encrypt);

This does a much better job of telling the reader what's going on than roter_1(true).

Don't repeat yourself

I didn't compare them carefully, but at least at first glance, it looks like roter_1, roter_2, roter_3, roter_4 and roter_5 are identical except for the contents of myArray_one each uses (and based on my recollection of how Enigma works, I believe that should be the case).

Assuming that's correct, it's probably preferable to have a single rotor function, with a parameter to tell it what array to use.

Use correct indentation

Indentation is normally used to reflect flow of control and (sometimes) continuation of the same statement). Indentation like this:

choiceforencr = "";
chosen_roter_one_s = "";
    chosen_roter_two_s = "";
    chosen_roter_three_s = "";
        currentLetter = "";
        encrypt_decrypt_output = "";
            chosen_roter_one = 0;
            chosen_roter_two = 0;
        chosen_roter_three = 0;
        rotate_position_one = 0;

..is not useful (and can be actively misleading).

\$\endgroup\$
26
\$\begingroup\$

This is not a standard header.

#include <conio.h>

Don't use it unless you really have to. The IO streams library is much better. If you must use it keep it separate from the other headers and comment why you need it.

Don't do this:

using namespace std;

See Why is “using namespace std” considered bad practice?. There is a reason the standard namespace is called std and not standard. Its so that using the prefix std:: is not that much of a burden.

Global variables are a code smell. Don't use them.

string option;
string choiceforencr;
string chosen_roter_one_s;
string chosen_roter_two_s;
string chosen_roter_three_s;
string currentLetter;
string encrypt_decrypt_output = "";
int chosen_roter_one;
int chosen_roter_two;
int chosen_roter_three;
int rotate_position_one;
int rotate_position_two;
int rotate_position_three;
int error = 0;
bool encrypt_Toggle = true;
int encr_decry_length;

You can use global read only variables to define constants but that's the only good use of a global. You should wrap your variables inside one ore more classes so you can control access to them.

An array of 1 character strings!!

        string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};

Why not just a string. Then you can use the character by indexing into the string?

        string myArray_one_decrypt = "zyxwvutsrqponmlkjihgfedcba";

This looks like it could be reduced considerably.

    if(currentLetter == "a"){currentLetter = myArray_one[0];}
    else if(currentLetter == "b"){currentLetter = myArray_one[1];}
    else if(currentLetter == "c"){currentLetter = myArray_one[2];}
    else if(currentLetter == "d"){currentLetter = myArray_one[3];}
     .....

There is no guarantee that characters are ASCII and thus monotonically increasing. But this is exceedingly common. You can add asserts to your code to enshrine the idea in the code. But you can convert a character to a number like this:

    int index = currentLetter - 'a';
    currentLetter = myArray_one[index];

Looks like the functions roter_1()/roter_2() ... roter_5() are practically identical. So this should be just one function. The state could then be incorporated into a class (and the function becomes a method). Then you can represents your roter's by an array of roter objects.

   std::array<Roter, 5>  roter;

This bit of code is repeated three times:

    if(chosen_roter_one == 1){
        final_Roter_one = roter_1;

    }
    else if(chosen_roter_one == 2){
        final_Roter_one = roter_2;

    }
    ....
    else{
        final_Roter_one = roter_5;

    }

Convert it to a function and return a result.

This is just really sloppy indenting.

        void updateStrings(string sentence){
            currentLetter = sentence[0];

        int stringLength = sentence.length();
        string wordLetters[stringLength];
        int initializer = 0;

        do{
        wordLetters[initializer] = currentLetter;
        currentLetter = sentence[initializer];
        initializer++;

At first I thought this was just all part of the class declaration. It took me a couple of takes to realize this was code. Readability is the key to coding. Learn a decent indent style and use it.

My chosen style:

        // Functions/Methods/Class definition
        Type name(Parameters)
        {
            allCodeHere();
            undented4Spaces();
        }

Inside a function I am more lax. I like to put '{' at the end of a line

        if (something) {
            doStuff();
        }
        for(int loop = 0; loop < 10; ++loop) {
            stuffInLoop(loop);
        }

Some extra vertical spacing to make the code readable will definately help here:

        if (stateof == 0){message = "What roters(1, 2, 3, 4, 5) would you like to use (You may only use 3):\n";}

And here:

        else{message = "Sorry your one of your last choices was not valid please try again\nWhat roters(1, 2, 3, 4, 5) would you like to use (You may only use 3):\n";}

Very rearly is it worth using std::endl.

      cout << "Your encrypted text is <<" << encrypt_decrypt_output << ">>" << endl;

Prefer "\n".

\$\endgroup\$
  • 4
    \$\begingroup\$ I chuckled at that’s why it’s called STD (emphasis mine) for obvious reasons \$\endgroup\$ – hyde Apr 27 '18 at 2:54
  • \$\begingroup\$ You only want std::endl instead of '\n' (or a trailing\n in a double-quoted string) if you want to flush the stream there, even if it's full-buffered, right? Or are there platforms where \n isn't equivalent? It compiles to more code than << '\n', and often you can include it as part of another fixed string. \$\endgroup\$ – Peter Cordes Apr 27 '18 at 17:25
  • \$\begingroup\$ @PeterCordes: Why would you want to flush the stream? The stream will flush itself when it needs to or when it is full. A programmer trying to flush the stream manually is usually doing it wrong and thus resulting in inefficient code. Note: std::cin forces std::cout to flush before accepting input. \$\endgroup\$ – Martin York Apr 27 '18 at 20:14
  • \$\begingroup\$ @PeterCordes: Sure combine \n into an existing string. Sure you can use '\n' rather than "\n" if you think it will make any difference in performance (I don't think there is a performance hit here). I prefer the double quotes because of consistency with the rest of the output which usually uses strings. \$\endgroup\$ – Martin York Apr 27 '18 at 20:16
  • \$\begingroup\$ Well usually yes you don't need to flush the stream early, so as you say it's very rarely worth using! But sometimes you want to write to a pipe more often. GNU grep has a --line-buffered option for interactive use when piping the output to something else. \$\endgroup\$ – Peter Cordes Apr 27 '18 at 20:17
8
\$\begingroup\$

You're using C++, but you only have one class in your code. Classes are intended to encapsulate the data and algorithms that represent something.

I'd take that a step further and say you have your use of classes backwards. Using a class to encapsulate a single algorithm with no data is pointless, you might as well just have a freestanding routine.

Meanwhile your roter_1() function tries to encapsulate the data and operation of a rotor in a function, and becomes really ugly because you're using the global rotate_position_one to store a value that should be a private member variable of the Rotor class.

Which in turn leads to bugs because it appears that due to your liberal use of copy-paste, all the roter_n() functions modify rotate_position_one. Any time you have that much duplicate code, you have a major problem, because a bug in that code needs to be fixed in five places, or eight for the naval machine

So I would see the class definition looking something like this:

class Rotor
{
public:
    Rotor(std:string const &wiring); // wiring is the "encryption" wiring of the rotor,
                                     // a string of the 26 letter of the alphabet in
                                     // the appropriate order for this rotor
    char Forward(char letter) const; // forward convert a letter: signal on the
                                     // way from the keyboard to the reflector
    char Reverse(char letter) const; // reverse convert a letter: signal on the
                                     // way from the reflector to the lamps
    bool Advance() const;            // Advance the rotor's position, return true if
                                     // and only if the next rotor to the left should
                                     // also advance
    ...
};

Then you just create three (or four for the navy version) of these, with the correct encryption strings for the constructor. You'll need some additional setup: either additional parameters to the constructor, or set by separate routines to define where the notch on the stepping ring is set to, whether it's a dual notch (naval) rotor, and the initial position of the rotor.

Do this, and now you only have one copy of the code, and the inner workings of the rotor are now kept safely hidden away from the rest of the code. All the rest of your program can do is access the rotor through a very well defined set of routines that do exactly what is needed.

You could also create a

class Reflector

and a

class Plugboard

to represent those two components of the machine. Reflector won't need Forward and Reverse functions, it'll just have a single

char Reflect(char letter) const;

function, however Plugboard will need both Forward and Reverse.

They both will need a way to set them up: the encryoption sequence for the reflector, and the plugboard wiring for that class.

Then assuming you put the rotors in an iterable container (e.g. std::vector) your encryption becomes:

for (char letter : message)
{
    letter = my_plugboard.Forward(letter);
    for (Rotor const &rotor : my_rotors)
    {
        letter = rotor.Forward(letter);
    }
    letter = my_reflector.Reflect(letter);
    // have to do this the hard way because C++ doesn't have a reverse ranged for.
    for (auto const rotor_it = my_rotors.crbegin(); rotor_it != my_rotors.crend(); rotor_it++)
    {
        letter = rotor_it->Reverse(letter);
    }
    letter = my_plugboard.Reverse(letter);
    // letter now contains the encrypted output - do something with it.

    // advance rotors
    for (Rotor &rotor : my_rotors)
    {
        if (!rotor.Advance();
        {
            break;
        }
    }
}

No guarantees this lot will compile, it's just typed in untested. However, the ideas in here should provide a framework that can be expanded into a fully functioning system.

\$\endgroup\$
  • 1
    \$\begingroup\$ ⟪have to do this the hard way because C++ doesn't have a reverse ranged for.⟫ You do the reversal in a range adaptor. In fact, this is the first example on the page I linked. Write: for(auto wheel : my_rotors|reverse) ⋯ \$\endgroup\$ – JDługosz Apr 28 '18 at 19:44
  • \$\begingroup\$ @JDługosz Good catch about the range adaptor. Using Boost is one way, and I've seen references to how to do it using nothing other than members of the std namespace. \$\endgroup\$ – dgnuff Apr 29 '18 at 0:24
6
\$\begingroup\$

I used the 1998 version of C++ instead of C++ 11 or C++ 14 because I needed access to the auto keyword to store the raw data of a function instead of the output.

That makes no sense.

I copied your code to

my machine. I see

auto final_Roter_one = roter_1;
auto final_Roter_two = roter_2;
auto final_Roter_three = roter_3;

which is global, meaning it cannot be the the pre-2011 meaning of auto. Since you are assigning the address of a function, it can’t be an implicit-int.

So, you are using the new C++11 syntax. This will not compile on an old compiler.

From these lines, I think I know what you meant by ⟪to store the raw data of a function instead of the output.⟫ You are assigning the address of the function to the variable.

Because of ancient history, it works as you have it, with the bare name of the function. But properly you should use the & operator.

auto final_Roter_one= &roter_1;

and to show that the auto is being used to deduce the type of the variable, the way you would write it before 2011 would be

int (final_Roter_one)() = &roter_1;

The Enigma machine did not have separate encrypt/decrypt modes. So I don’t know what you are doing with myArray_one_decrypt.


The main problem with your code is repetition. The idea of using a variable for each rotor and hooking them up in order by assigning to them is a sound idea (though they should be instances rather than functions). I call this out because it is a place where you did think abstractly, using variables to refer to different things at different times; the rest of your code is characterized by repetition rather than applying different data to a common piece of code.

Repetition: I see

string myArray_one[26] = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"};

in every function!

Your huge lists of if statements should use indexing instead of 26 lines.

auto i = currentLetter - 'a';
currentLetter = myArray_one[i];

But to get that to work so easily, you need to use char instead of string.


I don’t understand why you have 5 identical copies of the same function. In the Enigma Machine the rotors had different permutations wired up inside them.

Besides that, you should be able to have one copy of the code and logic, and just have a simple data table for each different rotor.

Hmm, I think you meant to replace rotate_position_one with a different name in each repetition of the function. Hmm hmm, no, you have one of those for each chosen rotor, not each wheel to choose from; or maybe that’s another mistake.

Instead of creating an array of 26 values and then rotating the array by a variable amount, every time the function is called, you can just add the position to the index.

auto i = currentLetter - 'a';
i = (i+rotate_position_one)%26;
currentLetter = myArray_one[i];

or something like that, depending on exactly how your mapping is meant to work.


Don’t give up! Keep learning and coding.

\$\endgroup\$
  • \$\begingroup\$ I was trying to be positive toward the OP, rather than just ripping everything he did. I agree it’s better if those were instances not functions, but it is a place where he did use indirection rather than a maze of if statements to produce the ordering. I guess no good deed goes unpunished. \$\endgroup\$ – JDługosz Apr 28 '18 at 19:36
  • \$\begingroup\$ That's fair. Can you do a quick edit to your post so I can change my vote? That said, while I agree that there is benefit in not "ripping it apart", there is something to be said for encouraging good habits right from the start. That's about what I'm trying to do in my answer: show what objects should be classes, and most important of all, why they should be. It's the "Algorithms and Data Structures" concept outlined in Wirth's book. A good rule to remember: a class makes the most sense when you have both. \$\endgroup\$ – dgnuff Apr 29 '18 at 0:07
  • \$\begingroup\$ Be careful assuming that 'a'...'z' are contiguous - although that's the case in ASCII and Unicode, it's not true of EBCDIC for example, and a strchr() would be a more portable version there. \$\endgroup\$ – Toby Speight Apr 30 '18 at 16:04
  • \$\begingroup\$ @TobySpeight you’re still assuming that the source code character set matches the runtime charset. And the machine's locale is set at run time, and the file you feed it can have some different encoding in any case. \$\endgroup\$ – JDługosz Apr 30 '18 at 23:39
4
\$\begingroup\$

Something that jumped out at me upon reading this question:

Also, would anyone happen to know how I could do decryption properly?

The Enigma machine works by swapping letters; it therefore has no separate mechanism for decryption, nor does it need one. Simply set up the machine identically as at the beginning of encryption, but enter the cyphertext instead of the plaintext.

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