Weird runtime error

[Apollyon]

Hi guys! I haven't been visiting this forum for nearly three years.

I was browsing some of my old programs, and I came across an Enigma rotor cipher simulator I coded. I re-compiled it, and now I have a runtime error I never had before:

It'll ask me whether I want to encrypt or decrypt, and let me enter the rotor settings, but then it won't let me enter the cleartext.

The code is attached. I hope I followed proper OO principles! It's probably some very simple error in main()...I hope.

Thanks!

[Paul McKenzie]

Did you attempt to debug your program? If not, why not, and if so, what are your observations?

Regards,

Paul McKenzie

[Paul McKenzie]

First, your code is not formatted correctly.

Second, your Rotor class is faulty. It has a pointer as a member variable, and this pointer points to dynamically allocated memory. However, your Rotor class has no user-defined copy constructor or assignment operator.

So I stopped right there. There is no need to go any further once a program uses classes coded this way.

Rotor r1;
Rotor r2 = r1;

A simple 2 line program would not work correctly.

Regards,

Paul McKenzie

[Paul McKenzie]

The same thing with the Enigma class. It isn't safely copyable or assignable. Also, why are you using "goto"? That alone could be an automatic candidate for disqualification for anyone looking at your code.

You included <string>, but you make no use of the std::string class. Doing that would alleviate the problems I pointed out. There is no "weirdness", as your thread title suggests, when you have code like this. It just isn't coded correctly and safely.

Regards,

Paul McKenzie

[Apollyon]

OK, I fixed all those problems (I wrote a cctor and assignment operator for both classes), and yes, I am using the string class, despite you saying I hadn't: look in the Enigma and main files.

I also indented the code so it's easier to read.

And, I got it to work!!! I needed to read up on C++ I/O streams.

Here's the new code.

[Paul McKenzie]

OK, I fixed all those problems (I wrote a cctor and assignment operator for both classes), and yes, I am using the string class, despite you saying I hadn't:

So why didn't you use the same std::string class in your Enigma and Rotor classes instead of char*? Then you wouldn't need to write copy constructor and assignment operator -- that's the real point I was trying to make.

Secondly, you did not actually write any implementation for the copy constructor or assignment operator.

#include <iostream>
#include <string>
using namespace std; 

class Rotor 
{ 
 public: 
  Rotor(char pos); 
  int GetSteps()const{return steps;} 
  static int CharacterMap(char the_char);
  void SetRotorPosition(int NewPos);
  void AdvanceRotor(int Steps);
  void ReverseRotor(int Steps);
  char GetCurrentCharacter()const;
  char GetCharacterIndex(int Index)const;
  char GetCharacterInverse(int i)const;

 private: 
  std::string Alphabet; 
  int CurPos; 
  int steps; 
};

static const char* alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; 

Rotor::Rotor(char pos=0):CurPos(0),steps(0), Alphabet(alphabet)
{
      SetRotorPosition(CharacterMap(pos)); 
}

#pragma once
#include "Rotor.hpp"
#include <string>

class Enigma 
{ 
 public: 
  Enigma(char r1,char r2,char r3,char r4,char r5);
  std::string Encrypt(const string& text); 
 private: 
  char plugboard(char Char);
  Rotor R1; 
  Rotor R2; 
  Rotor R3; 
  Rotor R4; 
  Rotor R5; 
  std::string Reflector;  
};

Enigma::Enigma(char r1,char r2,char r3,char r4,char r5): Reflector("COAYIWV7E3809TBUZ26NPGF4DQL5RJX1SHKM"),
         R1(r1),R2(r2),R3(r3),R4(r4),R5(r5)  { }

There is no need for dynamic memory allocation at all in any of your code -- instead a std::string is used.

And lastly, your "plugboard" function could have easily been written this way:

char Enigma::plugboard(char Char)
{
    static const char* returnData = "0QWEDTYUIOPSNMJKBZLFHXCVGRA987654321";
    if (Char >= 'A' && Char <= 'Z')
        return returnData[Char - 'A'];
    else
    if ( Char >= '0' && Char <= '9')
        return returnData[26 + Char - '0'];
   return ' ';
}

There was no need for endless if/else statements.

Regards,

Paul McKenzie

[Apollyon]

So why didn't you use the same std::string class in your Enigma and Rotor classes instead of char*? Then you wouldn't need to write copy constructor and assignment operator -- that's the real point I was trying to make.

Secondly, you did not actually write any implementation for the copy constructor or assignment operator.

#include <iostream>
#include <string>
using namespace std; 
 
class Rotor 
{ 
 public: 
  Rotor(char pos); 
  int GetSteps()const{return steps;} 
  static int CharacterMap(char the_char);
  void SetRotorPosition(int NewPos);
  void AdvanceRotor(int Steps);
  void ReverseRotor(int Steps);
  char GetCurrentCharacter()const;
  char GetCharacterIndex(int Index)const;
  char GetCharacterInverse(int i)const;
 
 private: 
  std::string Alphabet; 
  int CurPos; 
  int steps; 
};

static const char* alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; 
 
Rotor::Rotor(char pos=0):CurPos(0),steps(0), Alphabet(alphabet)
{
      SetRotorPosition(CharacterMap(pos)); 
}

#pragma once
#include "Rotor.hpp"
#include <string>
 
class Enigma 
{ 
 public: 
  Enigma(char r1,char r2,char r3,char r4,char r5);
  std::string Encrypt(const string& text); 
 private: 
  char plugboard(char Char);
  Rotor R1; 
  Rotor R2; 
  Rotor R3; 
  Rotor R4; 
  Rotor R5; 
  std::string Reflector;  
};

Enigma::Enigma(char r1,char r2,char r3,char r4,char r5): Reflector("COAYIWV7E3809TBUZ26NPGF4DQL5RJX1SHKM"),
         R1(r1),R2(r2),R3(r3),R4(r4),R5(r5)  { }

There is no need for dynamic memory allocation at all in any of your code -- instead a std::string is used.

And lastly, your "plugboard" function could have easily been written this way:

char Enigma::plugboard(char Char)
{
    static const char* returnData = "0QWEDTYUIOPSNMJKBZLFHXCVGRA987654321";
    if (Char >= 'A' && Char <= 'Z')
        return returnData[Char - 'A'];
    else
    if ( Char >= '0' && Char <= '9')
        return returnData[26 + Char - '0'];
   return ' ';
}

There was no need for endless if/else statements.

Regards,

Paul McKenzie

I made your requested changes. I also removed the Encrypt function, and overloaded the function call operator to do it's job. To make the code neater.

Also, did I follow proper OOP principles in designing my classes? I'd hate to see this written without using OOP; it'd likely be really long.

And since Rotors are only useful in Enigmas, should I make Rotor a private nested class of Enigma, so a Rotor cannot be instantiated out of context?

Anyway, thanks! Your advice was very helpful.