The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:
- You can enumerate the
PasswordGeneratoritself to get apasswordLengthnumber of chars. Why should someone do this instead of just getting a password string through theResultproperty? - The
Resultproperty magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property namedResult. "Result" of what? A better name would beNextorNextPassword. But a method would be more appropriate for this use case (e.g. namedCreateorGenerateorNext). - The most surprising thing, however, is the
GeneratePasswordsmethod. It is static, but you must pass it aPasswordGeneratoras argument. Why not make it an instance method then? Also, I would never guess that thesizeparameter would take chunks of this size from apasswordLengthpassword. E.g., if you initializepasswordLengthwith 16, thenResultreturns a password of this size, but if you callGeneratePasswordswith an argument of 5, it returns 3 passwords of length 5 and one with length 1. This does not make any sense.
Keep your public interface as simple as possible and use the Principle of least astonishment. It could be as simple as:
public class PasswordGenerator(int passwordLength, char[] legalChars) { /// <summary> /// Generates a new password at each call. /// </summary> public string Create() { ... } /// <summary> /// Generates 'count' passwords of length 'passwordLength'. /// </summary> public IEnumerable<string> Enumerate(int count) { ... } }- You can enumerate the
As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.