1
\$\begingroup\$

I made a password generator class in C++. I am not as familiar with C++ as I am with C, so I am looking for ways to move to a C++ mindset.

I am also trying to use embedded practices like avoiding allocating memory on the fly and other non-deterministic stuff.

I'll include the source file first, followed by the header, followed by the command line tool that is powered by this class.

passSingleton.cpp

#include <stdlib.h>     // atoi, rand
#include <iostream>     // cout, endl
#include <random>       // default_random_engine, uniform_int_distribution
#include <time.h>       // time

#include "passSingleton.h"  // class header

passSingleton::passSingleton()
{
    std::default_random_engine generator(time(NULL));
}

passSingleton::passSingleton(unsigned int passLength)
{
    passSingleton();
    setPasswordLength(passLength);
}

void passSingleton::generatePass()
/* Generates an ASCII password */
{
    int i;

    std::uniform_int_distribution<int> distribution(33,126);
    for(i = 0; i < passwordLength; i++) {
        password[i] = distribution(generator);  // ASCII chars between 33 - 126
    }
    password[i] = '\0';
}

int passSingleton::generatePass(unsigned int passLen)
{
    if(setPasswordLength(passLen))
        return -1;
    generatePass();
    return 0;
}

char* passSingleton::getPass()
{
    return password;
}

/* Sets password length to a positive int between 1 and MAX_PASS_LEN */
int passSingleton::setPasswordLength(unsigned int n)
{
    if (n < 8  || n > MAX_PASS_LEN) {
        std::cerr << "Password length must be between 8 and " << MAX_PASS_LEN << std::endl;
        exit(1);
    }
    passwordLength = n;
    return 0;
}

passSinglton.h

#ifndef PASS_SINGLE_H
#define PASS_SINGLE_H

#include <random>   // default_random_engine

class passSingleton
{
    public:
        passSingleton();
        passSingleton(unsigned int passLength);
        void generatePass();
        int generatePass(unsigned int passLen);
        char* getPass();
        int setPasswordLength(unsigned int n);
    private:
        static const unsigned int MAX_PASS_LEN = 128;
        char password[MAX_PASS_LEN];
        unsigned int passwordLength;
        std::default_random_engine generator;
};

#endif

main.cpp

#include <iostream>         // cout, endl, cerr
#include <stdlib.h>         // atoi
#include <unistd.h>         // getopt
#include <fstream>          // ofstream

#include "passSingleton.h"  // passSingleton class

void help();
void version();

int main(int argc, char** argv)
{
    char* myPass = NULL;
    char* filePath = NULL;
    passSingleton myPassFact;
    int c;
    unsigned int passLen = 8;
    unsigned int qty = 0;
    std::ofstream myFile;

    if (1 == argc) {
        std::cout << "Try `passgen -h' for more information" << std::endl;
        std::cout << "Usage: passgen -l PASSWORD_LENGTH" << std::endl;
        return 0;
    }

    /* Get command line options */
    while (-1 != (c = getopt(argc, argv, "hvl:f:q:"))) {
        int this_option_optind = optind ? optind : 1;
        switch (c) {
            case 'h':
                help();
                break;
            case 'l':
                passLen = atoi(optarg);
                if (0 == qty)
                    qty = 1;
                if (myPassFact.generatePass(passLen))
                    return -1;
                break;
            case 'v':
                version();
                break;
            case 'f':
                filePath = optarg;
                break;
            case 'q':
                qty = atoi(optarg);
                break;
            case '?':
                break;
            default:
                break;
        }
    }   

    /* Output block */
    if (NULL != filePath) { myFile.open(filePath); }
    while (0 != qty--) {
        myPassFact.generatePass();
        myPass = myPassFact.getPass();
        if (NULL != filePath)
            myFile << myPass << std::endl;
        else {
            std::cout << myPass << std::endl;
        }
    }
    if (NULL != filePath) { myFile.close(); }
    return 0;
}

void help()
{
    std::cout << \
    "Usage: passgen [OPTIONS] \n\
Generates an ASCII password PASSWORD_LENGTH characters long\n\
Example: passgen -l 32\n\
\n\
OPTIONS:\n\
    -l PASSWORD_LENGTH  specifies length of generated password\n\
    -f FILE         outputs password to specified file\n\
    -h          displays this help text\n\
    -v          displays version\n\
    -q QTY          generates QTY passwords" \
    << std::endl;
}

void version()
{

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. It seems like you're a beginner to C++ so you probably want to add beginner to the tags. Also, you seem to know some OOP from other languages, feel free to add your background into your question. However, please keep in mind that the code should not get changed, now that a review has been posted. \$\endgroup\$ – Zeta Jan 30 at 21:48
1
\$\begingroup\$

I would not make this to complicated. Your passSingleton::generatePass() method could simply return the password as a std::string with the password length as a parameter. Initialization of the random engine should be done from main.cpp and could also be passed as a parameter. Remove the class, keep the generatePass() method in main.

The check of the password length should generate an exception. If you cannot use exceptions, because of your embedded context, return a std::optional<std::string> if you're on C++17 or a std::tuple<bool, std::string> where the bool indicates success and the std::string contains the result.

When compiling, use at least -Wall it gives usefull hints on how to improve your code:

main.cpp:29:13: warning: unused variable 'this_option_optind'
      [-Wunused-variable]
        int this_option_optind = optind ? optind : 1;
            ^

Your application requires a minimum length of 8 characters, but if I run

sjank@WSDEBE16040-LXL> ./main -q 3 
!
-
h

the length defaults to 1. Is a minimum length really reasonable? You could give a warning instead. Use a sensible default length and make the -l parameter optional.

  • prefer #include <ctime> over #include <time.h>, remove <iostream> (see above) and <stdlib.h> (use <cstdlib> in c++).

  • Is writing passwords to a file is required? Is it best practice of storing passwords? Could the user achieve the same functionality otherwise (redirect)?

\$\endgroup\$
1
\$\begingroup\$
  • Option handling is questionable.

    First, do not hardcode the program name (passgen) in the message. Use argv[0] (if the executable gets renamed, the original message would lie).

    Second, give help immediately. Forcing the user to invoke the program one more time just to see the help is a nuisance. Consider

        if (1 == argc) {
            help();
            return 1;
        }
    

    Notice 1. Don't return 0 on error. Your program could be invoked from the script, and the script should be informed that something undesirable happened.

    Third, it is reasonable to expect that unrelated options could be given in any order. Your code behaves differently depending on the order of -l and -q. If -l is given first, the latter -q is ignored.

    Finally, the default case shall tell what option seems wrong, and also print the help.

  • File handling is scattered. Testing for NULL != filePath at three different points gives me shivers. Consider consolidating them either like

       if (NULL != filePath) {
           myFile.open(filePath, iOS_base::app);
           myFile << myPass << std::endl;
           myFile.close();
       } else {
           std::cout << myPass << std::endl;
       }
    

    (there is nothing wrong with opening the file multiple times), or freopen stdout. Don't forget to test the the file has been opened successfully.

    Better yet, always output to stdout, and let the user redirect as needed.

  • Don't use magic numbers (33,126). BTW, why you don't allow a space?

  • I understand the desire to avoid dynamic allocation. Just let the client provide the space for password(s).

  • C++ is not Java. Don't strive to put everything in a class. I see no reason for class passSingleton to exist.

\$\endgroup\$
0
\$\begingroup\$

I don't know where you are learning C++ from, but this what you posted is basically C with classes.


That being said, I am just going to point out in a list what you can improve. You can find enough material on the internet for each of the points.

  • Naming

    • Name your classes starting with an uppercase, e.g. passSingleton should be PassSingleton, otherwise it might look like a function call to the reader.
    • Name your classes, variables, etc. correctly, meaningfully, and written out, e.g. rename passSingleton to PasswordGenerator
    • Naming your generate function generatePass() is ambiguous (pass could be understood differently) and obsolete. Just name it generate() since your class is named PasswordGenerator.
  • Use STL containers!!!

    • instead of char password[MAX_PASS_LEN] use std::string password. With std::string you can still access each character with operator[].
    • You are not using any other containers here, but for future, use std::vector instead of raw C arrays.
  • In setPasswordLength() you are calling exit(1) if some requirements are not set. Don't exit from a function, rather return a bool or throw. This way, the caller of the function can still react.

  • constexpr

    • you can rewrite static const unsigned int MAX_PASS_LEN = 128; to static constexpr unsigned int MAX_PASS_LEN = 128;
  • just return the generated password in generate(). You don't need to save the password since you are not using it anywhere else. auto mypass = myPassFact.generatePass(); Doesn't this look way better?

  • In your passSingleton constructor, you are creating a local variable generator shadowing your member variable. Put the initialization of generator into the initializer list of the constructor. passSingleton() : generator(time(nullptr)) {}

  • nullptr instead of NULL

  • consider using Boost.Program Options instead of manually trying to parse the command line arguments
\$\endgroup\$

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.