Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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() and srand() methods to generate random letters.

I used dynamic allocation to create a char array based on user input. 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 user input.

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;
}
share|improve this question
    
You did it right. The "ask question" layout comes from the fact that this site is part of a Q&A network and this site is still using the default layout until the designers can get busy creating a custom one (and the powers that be decide this site is viable). –  ratchet freak Apr 24 at 19:59

1 Answer 1

A few things immediately jump out:

  1. Use std::string instead of char*.
  2. Your string is not null terminated (use of std::string fixes this)
  3. Don't create two globals when they're only used in one place.
  4. Don't create globals with an "uppercase" name. By common practice this is reserved for macros.
  5. srand() and rand() are superseded by any RNG from the <random> header.
  6. Don't use a do...while when a for-loop does the same job more succinctly.
  7. Your createPassword function does two things:

    1. It communicates with the user.
    2. It creates a new password.

    Split the logic into 2 functions.

share|improve this answer
    
main() doesn't require parameters per se. But yes, they are most recommended, especially if this will be run on a Unix system. Other than that minor thing, I agree with these things. –  Jamal Apr 24 at 20:16
    
Okay thank you very much! –  Marcus Head Apr 24 at 20:26
    
I have removed the item with regards to the function prototype of main(). The C++ standard (section 3.6.1.) specifies both (xor) uses are allowed. –  Gerard Apr 24 at 20:32
    
Uppercase names are not "reserved" for macros. That's merely a common convention. –  Edward Apr 24 at 20:34
    
@Edward Agreed. I have updated the corresponding item to reflect this. –  Gerard Apr 24 at 20:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.