Console based password generator

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
#include
#ifdef WIN32
#include
#else
#include
#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.");

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 `:

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 tm_year + 1900) tm_mon+1) tm_mday tm_hour tm_min tm_sec 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 tm_year + 1900) tm_mon+1) tm_mday tm_hour tm_min tm_sec 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 tm_year + 1900) tm_mon+1) tm_mday tm_hour tm_min tm_sec 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 `:

std::random_device rd;
std::mt19937 g(rd());

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

StackExchange Code Review Q#85223, answer score: 6