8
\$\begingroup\$

I have written an extremely simple program that has rudimentary password generation with selective character types (symbols, numbers, uppercase, and lowercase). Here is the C++ code:

#include <iostream>
#include <cstdlib>
#include <ctime>

inline bool getOptions(const std::string & option);
inline long long unsigned getLength();

int main(){
    bool syms   = getOptions("Symbols"),
        nums    = getOptions("Numbers"),
        lower   = getOptions("Lowercase Chars"),
        upper   = getOptions("Uppercase Chars");

    if(!(syms || nums || lower || upper)){
        std::cout << "ERR: No options selected.";
        return -1;
    }

    const std::string str_syms = "`~!@#$%%^&*()-=_+[]{}\\|;:\'\",<.>/\?",
                str_nums = "1234567890",
                str_lower = "abcdefghijklmnopqrstuvwxyz",
                str_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

    std::string allowedChars;

    if(syms) allowedChars += str_syms;
    if(nums) allowedChars += str_nums;
    if(lower) allowedChars += str_lower;
    if(upper) allowedChars += str_upper;

    long long unsigned passLength = getLength();

    std::string password;
    srand(time(NULL));
    for(long long unsigned i = 0; i < passLength; ++i){
        password += allowedChars[std::rand() % allowedChars.length()];
    }

    std::cout << password << '\n';

    return 0;
}

inline bool getOptions(const std::string & option){
    std::string choice;
    while(true) {

        std::cout << option + " (y/N): ";

        std::getline(std::cin, choice);

        if(tolower(choice[0]) == 'y') {
            return true;
        } else if(tolower(choice[0]) == 'n') {
            return false;
        }

    }
}

inline long long unsigned getLength(){
    std::string str_passLength;
    long long unsigned passLength;
    while(true) {
        std::cout << "Password Length: ";
        std::getline(std::cin, str_passLength);

        try{
            passLength = std::stoll(str_passLength);
        } catch(const std::invalid_argument & e){
            std::cerr << "\n\tERR: Invalid argument.\n";
        } catch(const std::out_of_range & e){
            std::cerr << "\n\tERR: Out of range (long long unsigned).\n";
        } catch(...){
            std::cerr << "\n\tERR: Unknown, something messed up.\n";
        }

        if(passLength > 0){
            return passLength;
        }
    }
}

Then, I recreated the program in Python3, just for practice since I am learning the syntax of Python.

#!/usr/bin/python

from random import randint

def getOptions(option):
    choice = ""
    while True:

        print("{} (y/N): ".format(option), end="")
        choice = input()

        if choice[0].lower() == 'y':
            return True
        elif choice[0].lower() == 'n':
            return False

def getLength():
    str_passLength = ""
    passlength = 0
    while True:

        str_passLength = input("Password Length: ")

        try:
            passlength = int(str_passLength)
        except:
            print("\n\tERR: Invalid.\n")

        if passlength > 0:
            return passlength

def main():
    syms    = getOptions("Symbols")
    nums    = getOptions("Numbers")
    lower   = getOptions("Lowercase Chars")
    upper   = getOptions("Uppercase Chars")

    if not(syms or nums or lower or upper):
        print("ERR: No options selected.")
        return -1

    str_syms = "`~!@#$%%^&*()-=_+[]{}\\|;:\'\",<.>/\?"
    str_nums = "1234567890"
    str_lower = "abcdefghijklmnopqrstuvwxyz"
    str_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"

    allowedChars = ""

    if syms: allowedChars += str_syms
    if nums: allowedChars += str_nums
    if lower: allowedChars += str_lower
    if upper: allowedChars += str_upper

    passLength = getLength()

    password = ""

    for i in range(passLength):
        password += allowedChars[randint(0, len(allowedChars) - 1)]

    print(password, '\n')

main()

I understand that this program is so short that all realistically long password lengths will run almost instantaneously on all modern machines.

However, I wish to improve its readability, logic, and most importantly its brevity. How can I do so?

\$\endgroup\$
11
\$\begingroup\$

In both languages:

  • Why does your syms contain '%' twice and not contain an escaped '\\' at all? (Notice that '\?' is equivalent to '\\?' in Python, but I would hate to ship code relying on that.)

In Python:

  • Look up argparse and use it. This will cut down your "option parsing" code to near zero lines. (There's no standard equivalent in C++; but it'll be good if you can adopt similar control flow in C++. Mixing I/O with business logic is never a good thing.)

  • allowedChars[randint(0, len(allowedChars) - 1)] is a verbose way of saying random.choice(allowedChars).

  • At the bottom of your program, you should insert if __name__ == '__main__': in front of the call to main(), so that you can import this module from elsewhere without accidentally calling main() in the process.

In C++:

  • You repeat the keyword inline as if it means something in this context. Don't. Save inline for contexts where it actually has a meaningful effect on linkage (as in, where the program wouldn't compile without it).

  • Don't declare multiple variables in a single statement; e.g. replace bool x = a, y = b, z = c; with bool x = a; bool y = b; bool z = c;.

  • std::rand() is not a blessed way to get random bits in C++. Look up std::random_device and std::uniform_int_distribution. Admittedly the currently blessed way to get random bits is a bit verbose, but it's probably worth learning at some point.

  • Nit: You're missing at least #include <string> and #include <cctype>.

\$\endgroup\$
  • 2
    \$\begingroup\$ Since you are proposing more cryptographically secure approaches in C++, you should suggest to use the secrets module in Python as well. \$\endgroup\$ – 301_Moved_Permanently Apr 23 '17 at 12:47
  • \$\begingroup\$ Ah, as far as the double % goes, I must've mistakenly used the notation for escaping % in C's printf statements. I do believe that I have an escaped '\\' in syms, it is my understanding that it is right before the | character. \$\endgroup\$ – esote Apr 23 '17 at 19:42
3
\$\begingroup\$

The first thing I'd suggest is to separate the user interactions and the code logic. A class for the actual password generation would allow you to only expose those parts that the user would need to access.

Also instead of several string concatenations, it seems to me that setting the maximum size of the string, copying the ones you want then resizing would probably be more efficient.

Here's my take on how it would look:

class PasswordChooser
{
    const string symbols = "`~!@#$%^&*()-=_+[]{}|;:\'\",<.>/\?";
    const string numbers = "1234567890";
    const string lowerCase = "abcdefghijklmnopqrstuvwxyz";
    const string upperCase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    string pool;
    int totalLength = 0;
    bool poolCreated = false;

public:
    PasswordChooser( )
    {
    }

    string CreatePassword(int length,bool duplicates = true )
    {
        if ( !poolCreated )
        {
            return "Create pool first";
        }

        if ( length > pool.size() && !duplicates )
        {
            return "Length is too long.  Create a longer pool or change options";
        }
        string retVal;
        retVal.resize( length );
        for ( int i = 0; i < length; i++ )
        {
            int index = rand() % pool.size();
            retVal[i] = pool[index];
            if ( !duplicates )
            {
                pool.erase( pool.begin() + index);
            }
        }

        poolCreated = false;
        return retVal;
    }
    void CreatePool( bool hasSymbols = false , bool hasNumbers = false , bool hasLowerCase = false , bool hasUpperCase = false )
    {
        pool = string( symbols.length() + numbers.length() + lowerCase.length() + upperCase.length(),'\0' );
        totalLength = 0;
        if ( hasSymbols )
        {
            copy( symbols.begin() , symbols.end() , pool.begin() + totalLength );
            totalLength += symbols.length();
        }
        if ( hasNumbers )
        {
            copy( numbers.begin() , numbers.end() , pool.begin() + totalLength );
            totalLength += numbers.length();
        }
        if ( hasLowerCase )
        {
            copy( lowerCase.begin() , lowerCase.end() , pool.begin() + totalLength );
            totalLength += lowerCase.length();
        }
        if ( hasUpperCase )
        {
            copy( upperCase.begin() , upperCase.end() , pool.begin() + totalLength );
            totalLength += upperCase.length();
        }
        pool.resize( totalLength );
        if ( pool.size() == 0 )
        {
            poolCreated = false;
        }
        else
        {
            poolCreated = true;
            random_shuffle( pool.begin() , pool.end() );
        }
    }
};
\$\endgroup\$
2
\$\begingroup\$

I would make the logic in your Python main function slightly shorter by using what @Quuxplusone already suggested in his answer. But I would go further and make a get_password function that takes the requested length and the allowed characters as arguments. It uses str.join and random.choice. It also uses _ as name for an unused loop variable, as is customary in Python.

In addition, I implemented the argparse you would need, including the validation step that at least one charset should be used.

I made the charset a list, because extending lists is slightly faster than string addition, even though it probably won't matter in this small case.

Python also has a built-in string module that pre-defines a lot of commonly used character classes (and it has all of the ones you use).

I also renamed your variables according to Python's official style-guide, PEP8.

import argparse
import random
import string


def parse_args(args=None):
    parser = argparse.ArgumentParser()
    parser.add_argument("n", type=int, default=8, help="Length of password")
    parser.add_argument("--syms", action='store_true')
    parser.add_argument("--nums", action='store_true')
    parser.add_argument("--lower", action='store_true')
    parser.add_argument("--upper", action='store_true')

    args = parser.parse_args(args)
    if not (args.syms or args.nums or args.lower or args.upper):
        parser.error('No character options selected')

    return args


def get_password(n, allowed_chars):
    return "".join(random.choice(allowed_chars) for _ in range(n))


def main():
    # use argparse module to get arguments
    args = parse_args()

    allowed_chars = []
    if args.syms:
        allowed_chars.extend(string.punctuation)
    if args.nums:
        allowed_chars.extend(string.digits)
    if args.lower:
        allowed_chars.extend(string.ascii_lowercase)
    if args.upper:
        allowed_chars.extend(string.ascii_uppercase)

    # Use a password generating function
    print(get_password(args.n, allowed_chars))


if __name__ == "__main__":
    main()

Side note: I made the parse_args function accept an argument args, so I can easily test it. By default argparse tries to parse sys.argv, but if args is not None, it will take a list of strings. So you can test it with e.g. args = parse_args(["6", '--lower', '--upper']).

\$\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.