Skip to main content
added 100 characters in body
Source Link
  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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)
    {
        public string Create()
        {
            ...
        }
    
        public IEnumerable<string> Enumerate(int count)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

added 100 characters in body
Source Link
  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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)
    {
        public string Create()
        {
            ...
        }
    
        public IEnumerable<string> Enumerate(int passwordLengthcount)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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(char[] legalChars)
    {
        public string Create(int passwordLength)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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)
    {
        public string Create()
        {
            ...
        }
    
        public IEnumerable<string> Enumerate(int count)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

Source Link

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with 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(char[] legalChars)
    {
        public string Create(int passwordLength)
        {
            ...
        }
    }
    
  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.