Review my Source Code

[MarkyMark]

This program is a random password generator. It asks the user how many chars they want their password to be and how many passwords it should generate.
I used the rand() srand() method to generate random letters.
I used dynamic allocation to create a char array based on userinput.
The comment line is where I tried to dynamically allocate an array of pointers, where each pointer would point to a char array, but I couldn't figure out how to do it so I used a counter and a do while loop and a for loop to create passwords based on userinput.
Please give me advice on how I can improve or what I can do differently!

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

const int MAX = 90;
const int MIN = 65;

char * createPassword();

int main()
{
	char * p = createPassword();	

	

	
	return 0;
}

char * createPassword()
{
	unsigned seed = time(0);

	srand(seed);
	
	char x = ' ';
	int passwordLength = 0;
	int numOfPasswords = 0;

	std::cout << "How many chars in password: ";
	std::cin >> passwordLength;
	char * pwptr = new char[passwordLength];

	std::cout << "How many passwords should be generated?";
	std::cin >> numOfPasswords;
	//char * passwords = new char *pwptr[numOfPasswords];

	int passwordcount = 0;

	do{
		for(int cnt = 0; cnt < passwordLength; cnt++)
		{
			x = (rand() % (MAX - MIN + 1)) + MIN;
			pwptr[cnt] = x;
			std::cout << pwptr[cnt];
		}
		std::cout << std::endl;
		passwordcount++;
		} while(passwordcount != numOfPasswords);

		return pwptr;
}

[Disch]

Aside from the memory leak it's not bad. I've seen much worse. =)


Here are some suggestions for improvement:

1) you should not srand() inside createPassword. You should do it once at program start. So lines 23,25 should probably be moved to the start of main.

2) Don't use new[]. Since you are never delete[]'ing the memory, you are leaking it. If you need a dynamically sized string, use the string class. It saves you the trouble of having to dynamically allocate and destroy your memory.

3) If you want an array (like an array of strings), you can use vector. A vector<string> would be able to hold all the generated passwords. But you don't really need that anyway, since all you're doing with these passwords is printing them.

4) createPassword should do 1 thing: create a single password. It should not do any I/O. You have it doing several things:
- ask for password length
- ask for number of passwords
- create several passwords
- print all the passwords

This is too much work for this function. Furthermore, you return pwptr... but why? At best it will only be the last password created, but no outside code would have any use for it because you've already printed the passwords.

If createPassword needs information to do its work, that information should be provided through function parameters -- it should not poll the user.

5) Your do/while loop is mimicing a for loop -- but is less clear. For situations where you are looping a fixed number of times, just use a for loop.

6) Don't use magic numbers. Your min/max values of 65 and 90 seem to correspond to ASCII values of 'A' and 'Z'. So instead of memorizing the numerical values for those characters and confusing your code... just use 'A' and 'Z': const int MIN = 'A';

7) If I can get really nitpicky -- you shouldn't be using srand/rand at all, but instead should be using C++11 <random> header functionality.

8) I just realized you are not null terminating your character array, so that would be problematic if you ever actually used pwptr.


Here's a rewrite with comments so you can see what I did:

#include <random>
#include <ctime>
#include <string>
#include <iostream>

// This is the RNG object.  It will actually create random numbers.  I'm making it global just because it's easier
//   for this example.
std::mt19937    rnge( (unsigned)time(nullptr) );        // <- seed it with the current time, just like srand


// This function will create a random password with the given length, and will return that password
//   as a string
std::string createPassword(int length)
{
    // This is a "distribution".  Basically this is used with the above 'rnge' engine to produce a
    //   random number in the given range.
    std::uniform_int_distribution<int>      dis('A', 'Z'); // <- only produce integers between ['A','Z']

    std::string pw;  // our password.  This will be returned once we populate it.

    for(int i = 0; i < length; ++i)
    {
        pw.push_back( (char)dis(rnge) );// this will generate a random character between 'A' and 'Z' and will append it to our
                                        //   pw string.  A longer way to write this would be:
        /*
        char x = (char)dis(rnge);   // generate the random character
        pw.push_back( x );          // add that character to the end of our string
        */
    }

    return pw;  // return our password now that it is built.
}


//  Main
int main()
{
    int numPasswords;
    int passwordLength;

    // get some crap from the user
    std::cout << "How many passwords do you want to generate?: ";
    std::cin >> numPasswords;

    std::cout << "How many characters in each password?: ";
    std::cin >> passwordLength;

    std::cout << "\n\nGenerated passwords:\n";
    std::cout <<     "--------------------\n";

    for(int i = 0; i < numPasswords; ++i)
    {
        std::cout << createPassword(passwordLength) << '\n';
    }

    std::cout << "\nDone!" << std::endl;

    system("pause");
}

[Disch]

again note that every new[] must have a delete[]. So with this "nested new" approach where you create a dynamic array with new[] inside of another new[], you will also have to delete[] inside of another delete[].

Seriously -- just don't use new[]. It creates error prone and hard-to-use code. If you want an array of strings, use a vector<string>.

[MarkyMark]

Wow! Thank you so much! I'm glad you took the time to help me out, thanks for the critique! A lot of good info here :D