8
\$\begingroup\$

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()
{
}
\$\endgroup\$
  • \$\begingroup\$ Why isn't oFstream -> oOfstream ? \$\endgroup\$ – bhathiya-perera Mar 28 '15 at 2:57
  • 1
    \$\begingroup\$ A good read Passphrases That You Can Memorize — But That Even the NSA Can’t Guess \$\endgroup\$ – Martin York Mar 28 '15 at 4:04
  • \$\begingroup\$ Can you add the declaration of Controller::PwGenerator and Utility::Security without them a code review is pointless. It would also be good to see a main() to show a usage example. \$\endgroup\$ – Martin York Mar 28 '15 at 4:10
  • \$\begingroup\$ @JaDogg Thanks. In my local source code this is changed yet. [at]Loki Astari Thanks. I will look into the shared link. I have added the header file of Controller::PwGenerator & Utility::Security but also main.cpp to my question. \$\endgroup\$ – AMartinNo1 Mar 28 '15 at 10:41
6
\$\begingroup\$

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);
    
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I will use c++14 then. About the prefix this->. I like to use it as I know immediately that it is a global var. I can live with longer source code or does it impact the performance or has any other bad influence? The reason I declare vars in the beginning of a function is that I have experienced a couple of times that variables were needed later time outer if-statements, loops etc. As my functions are not that long normally I find the usage easily. If it's a bad habit I ask you to give another example why it's bad. \$\endgroup\$ – AMartinNo1 Mar 28 '15 at 11:06
  • \$\begingroup\$ I will remove the useless pointer as suggested by you and use the new shuffle function. \$\endgroup\$ – AMartinNo1 Mar 28 '15 at 11:08
  • \$\begingroup\$ @AMartinNo1 Declaring the variables in the tighest scope possible is something that will tell to people who read your code (or yourself in a few months): "great, that variable isn't used before nor later" compared to "oh, where did I declare this variable? Is it used before? Is it used after?". I guess that it may help the compiler produce better code too since it will know that it can get rid of a variable sooner. \$\endgroup\$ – Morwenn Mar 28 '15 at 11:48
  • \$\begingroup\$ Hm. yeah that makes sense indeed :) \$\endgroup\$ – AMartinNo1 Mar 28 '15 at 12:29
  • \$\begingroup\$ I update the source code now except of c++14 stuff as I don't have the time to figure out how to compile c++14. \$\endgroup\$ – AMartinNo1 Mar 28 '15 at 14:18
3
\$\begingroup\$

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).
\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. 1. I am fine with it. 2. Thanks for the hint. I will change that. 3. Same as second. 4. Same as second. You're right its senseless to randomly abbreviate vars. \$\endgroup\$ – AMartinNo1 Apr 3 '15 at 9:03

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.