Weird runtime error
CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Results 1 to 7 of 7

Thread: Weird runtime error

  1. #1
    Join Date
    Oct 2004
    Location
    Long Beach, CA
    Posts
    179

    Weird runtime error

    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!
    Attached Files Attached Files
    Last edited by Apollyon; July 2nd, 2010 at 10:39 PM.
    The nerds will rule over the common folk. The geeks will rule over the nerds. The supergeeks will rule over the geeks. The hackers will rule over the supergeeks. The superhackers will rule over the hackers. Mwhahaha; I'll rule you all!!!

  2. #2
    Join Date
    Apr 1999
    Posts
    27,446

    Re: Weird runtime error

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

    Regards,

    Paul McKenzie

  3. #3
    Join Date
    Apr 1999
    Posts
    27,446

    Re: Weird runtime error

    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.
    Code:
    Rotor r1;
    Rotor r2 = r1;
    A simple 2 line program would not work correctly.

    Regards,

    Paul McKenzie

  4. #4
    Join Date
    Apr 1999
    Posts
    27,446

    Re: Weird runtime error

    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

  5. #5
    Join Date
    Oct 2004
    Location
    Long Beach, CA
    Posts
    179

    Re: Weird runtime error

    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.
    Attached Files Attached Files
    The nerds will rule over the common folk. The geeks will rule over the nerds. The supergeeks will rule over the geeks. The hackers will rule over the supergeeks. The superhackers will rule over the hackers. Mwhahaha; I'll rule you all!!!

  6. #6
    Join Date
    Apr 1999
    Posts
    27,446

    Re: Weird runtime error

    Quote Originally Posted by Apollyon View Post
    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.
    Code:
    #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; 
    };
    Code:
    static const char* alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; 
    
    Rotor::Rotor(char pos=0):CurPos(0),steps(0), Alphabet(alphabet)
    {
          SetRotorPosition(CharacterMap(pos)); 
    }
    Code:
    #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;  
    };
    Code:
    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:
    Code:
    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
    Last edited by Paul McKenzie; July 3rd, 2010 at 06:09 AM.

  7. #7
    Join Date
    Oct 2004
    Location
    Long Beach, CA
    Posts
    179

    Re: Weird runtime error

    Quote Originally Posted by Paul McKenzie View Post
    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.
    Code:
    #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; 
    };
    Code:
    static const char* alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; 
     
    Rotor::Rotor(char pos=0):CurPos(0),steps(0), Alphabet(alphabet)
    {
          SetRotorPosition(CharacterMap(pos)); 
    }
    Code:
    #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;  
    };
    Code:
    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:
    Code:
    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.
    Last edited by Apollyon; July 3rd, 2010 at 09:37 PM.
    The nerds will rule over the common folk. The geeks will rule over the nerds. The supergeeks will rule over the geeks. The hackers will rule over the supergeeks. The superhackers will rule over the hackers. Mwhahaha; I'll rule you all!!!

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  


Windows Mobile Development Center


Click Here to Expand Forum to Full Width

This is a CodeGuru survey question.


Featured


HTML5 Development Center