Console based password generator

Question

I am currently developing a password generator and just finished my first version. I know there is a lot to do but before I proceed with the next version I would like to have feedback on my current source code.

GitHub

The embedded source code does not contain comments to reduce characters.

Main.cpp

void main()
{
    Controller::PwGenerator* oController = new Controller::PwGenerator();
    oController->Run();
    delete oController;
}

---

PwGenerator.h

#pragma once
namespace View
{
    class Terminal;
}
namespace Utility
{
    class Security;
}
namespace Controller
{
    class PwGenerator
    {
        private:
            View::Terminal* oTerminal;
            Utility::Security* oSecurity;
        public:
            PwGenerator();
            void Run();
            ~PwGenerator();
    };
}

PwGenerator.cpp

#include "PwGenerator.h"
#include "Terminal.h"
#include "Security.h"
#include <fstream>
#include <ctime>
#ifdef WIN32
#include <windows.h>
#else
#include <unistd.h>
#endif
Controller::PwGenerator::PwGenerator()
{
    this->oTerminal = new View::Terminal();
    this->oSecurity = new Utility::Security();
}
void Controller::PwGenerator::Run()
{
    bool bFinish = false;
    int iMenuItem = 1;
    std::ofstream oFstream;
    std::time_t oTime = time(0);
    std::tm* timePtr = std::localtime(&oTime);
    this->oTerminal->Programminfo();
    #ifdef WIN32
    Sleep(3000);
    #else
    usleep(3000 * 1000);
    #endif
    do
    {
        this->oTerminal->Menu();
        iMenuItem = this->oTerminal->SelectMenuOption();
        switch (iMenuItem)
        {
            case 0:
                bFinish = true;
            break;
            case 1:
                this->oTerminal->Programminfo();
            break;
            case 2:
                this->oSecurity->GeneratePw( this->oTerminal->MenuePwGen() );
                if (this->oTerminal->MenueSavePw() == true)
                {
                    this->oTerminal->Print("The password has been saved in a file.");
                    oFstream.open("PasswordGenerator.txt", std::fstream::app);
                    oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << this->oSecurity->GetPw();
                    oFstream.close();
                }
                else
                {
                    this->oTerminal->Print("Generated password:");
                    this->oTerminal->Print( this->oSecurity->GetPw() );
                }
            break;
            default:
            break;
        }
    } while (bFinish == false);
}
Controller::PwGenerator::~PwGenerator()
{
    delete this->oTerminal;
}

---

Security.h

#pragma once
#include <string>
namespace Utility
{
    class Security
    {
        private:
            std::string sPassword;
            std::string sAllowedCharacter;
        public:
            Security();
            void GeneratePw(int iLength);
            std::string GetPw();
            ~Security();
    };
}

Security.cpp

Utility::Security::Security()
{
    this->sPassword = std::string("");
    this->sAllowedCharacter =      std::string("abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXY0123456789!$%&()[]{}=?");
}
void Utility::Security::GeneratePw(int iLength)
{
    this->sPassword = this->sAllowedCharacter;
    std::random_shuffle(this->sPassword.begin(), this->sPassword.end());
    this->sPassword = this->sPassword.substr(0, iLength);
    std::random_shuffle(this->sPassword.begin(), this->sPassword.end());
}
std::string Utility::Security::GetPw()
{
    return this->sPassword;
}
Utility::Security::~Security()
{
}

Answer 1

It seems that you could use a little C++11 in order to improve your code:

• You can abstract away the operating system you are using by replacing the platform-dependent sleep functions by std::this_thread::sleep_for. If you can use C++14, you can even benefit from the added readability gained with the standard user-defined literals in <chrono>:

  using namespace std::chrono_literals;
  std::this_thread::sleep_for(3s);
  

• You can avoid using most of the this-> you are using. Generally speaking, you don't need to prepend this->: it just makes the code longer and the compiler should warn you anyway if a local variable shadows a class member (-Wshadow with GCC for example).

• Declaring all the variables at the beginning of a function is very C-like and makes it hard to know where a variable is used exactly. Generally speaking, you should only declare variables when you need them, in the tightest possible scope. Have a look at this piece of your code:

  if (this->oTerminal->MenueSavePw() == true)
  {
      this->oTerminal->Print("The password has been saved in a file.");
      oFstream.open("PasswordGenerator.txt", std::fstream::app);
      oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << this->oSecurity->GetPw();
      oFstream.close();
  }
  

  Here, you can clearly declare oFstream in the if block since it isn't used before nor after. Moreover, it allows you to open the stream at construction and to let the end of the scope call the destructor, which will call close for you:

  if (oTerminal->MenueSavePw() == true)
  {
      oTerminal->Print("The password has been saved in a file.");
      std::ofstream oFstream("PasswordGenerator.txt", std::fstream::app);
      oFstream << "\r\n[" << (timePtr->tm_year + 1900) << "." << (timePtr->tm_mon+1) << "." << timePtr->tm_mday << " " << timePtr->tm_hour << ":" << timePtr->tm_min << ":" << timePtr->tm_sec << "] " << oSecurity->GetPw();
  }
  

• By the way, it is considered bad practice to compare conditions against true. You should simply change it to:

  if (oTerminal->MenueSavePw())
  

• Your lines are quite long too. You could split them into several lines to make your code more readable. Consider the following and compare it to your one-liner:

      oFstream << "\r\n["
               << (timePtr->tm_year + 1900) << "."
               << (timePtr->tm_mon+1) << "."
               << timePtr->tm_mday << " "
               << timePtr->tm_hour << ":"
               << timePtr->tm_min << ":"
               << timePtr->tm_sec << "] "
               << oSecurity->GetPw();
  

  Sometimes, when printing things that have a structure (like printing XML data), you can even use fancier indentation in order to reflect the structure of the data you're printing.

• There are too many useless pointers and dynamic allocation in your code. In C++, whenever possible, you should use automatic variables instead of dynamically allocated ones. Here is what your main should look like:

  void main()
  {
      Controller::PwGenerator oController;
      oController.Run();
  }
  

  Terser, safer and even probably faster (unnoticeable in this case though). Whenever you use new, your program may throw an std::bad_alloc, and dynamically allocated memory doesn't free itself when an exception occurs while automatic variables are correctly destructed when an exception is thrown. Bottom line: use pointers when you have to and regular variables whenever you can.

• I said that this is what main should look like, but it's actually not true: your main has a void return type, which makes the program ill-formed in regards to the standard. While most compilers will accept it, you better have an int return type for your main to be standard-compliant. You don't have to return anything from it though since it is guaranteed that the compiler will add a return 0; at the end of it if it didn't encounter any return statement.

• std::random_shuffle has been deprecated in C++14 and will be removed from the standard library in C++17 because it might be unsafe. You should consider using std::shuffle instead, with a random number generator from the C++11 header <random>:

  std::random_device rd;
  std::mt19937 g(rd());
  
  std::shuffle(sPassword.begin(), sPassword.end(), g);
  

Answer 2

A few minors:

1. There are many good reason not to use hungarian notation.
2. In Security the member sAllowedCharacter should be
   • named sAllowedCharacters (plural)
   • const because it's never changing.

3. Your method namings are a bit weird. For example reading this line:

   this->oSecurity->GeneratePw( this->oTerminal->MenuePwGen() );
   

   I have no idea why the output of MenuePwGen() is passed to GeneratePw(). If it were named GetPasswordLength() it would be a lot clearer and it would also describe a lot better what the point of the particular menue point is.

4. Again related to naming - please don't randomly abbreviate things. It doesn't huert to name methods GeneratePassword and the code will be a lot easier to read. Auto-completion is a standard feature these days for any decent development environment (in case you're worried about the extra typing).