I just wrote my first Java class, and I wanted to share it with you and maybe get some quick check from more experiences coders than I am.
I want to learn OOP, and Java is just the tool I thought was the best to start with (it's very similar to C in syntax, which I'm used to, and it's spreading more and more, day by day).
You can obviously comment on Java bad habits and ways to improve the class, but consider the class structure and the abstraction of the problem I wanted to solve, the separation between in data methods, etc.. as my main point of concern.
Here's the class. The aim is to create a random password generator.
import java.util.Random;
public final class PasswordGenerator {
// DATAS
// characters with which the password will be composed
private static final int charactersSize = 100;
private static char [] characters = new char [charactersSize];
// keep the counts of used characters
private static int charactersCount = 0;
// size of the password to generate
private int passwordSize;
// CONSTRUCTOR
public PasswordGenerator( int passwordSize ) {
// set the password size
this.passwordSize = passwordSize;
// set the characters that will be used to generate the password
initCharacters();
}
// METHODS
// fill the array of characters that will be used to generate the password
private static char [] initCharacters() {
int i = 0;
// add 0-9
for ( int j = 48; j < 58; ++i, ++j, ++charactersCount ) {
characters[i] = (char) j;
}
// add @ + a-z
for ( int j = 64; j < 91; ++i, ++j, ++charactersCount ) {
characters[i] = (char) j;
}
// add A-Z
for ( int j = 97; j < 123; ++i, ++j, ++charactersCount ) {
characters[i] = (char) j;
}
return characters;
}
// generate a random password
public char [] get() {
// initialize the random number generator
Random rnd = new Random();
char [] password = new char [passwordSize];
// choose a random character from the array
for ( int i = 0; i < passwordSize; ++i ) {
password[i] = characters[ rnd.nextInt(charactersCount) ];
}
return password;
}
// DEBUG METHODS
// show the characters the will be used to compose the pass
public void showCharacters() {
for ( int i = 0; i < charactersCount && characters[i] != 0; ++i ) {
System.out.println(characters[i]);
}
}
// MAIN - testing code
public static void main(String[] args) {
int passwordSize = 10;
PasswordGenerator password = new PasswordGenerator( passwordSize );
System.out.println( password.get() );
}
}
-
3\$\begingroup\$ From a security perspective, I hope you're not going to use Random for real passwords. It produces low-quality random numbers. You should use SecureRandom instead for actual applications such as this. \$\endgroup\$user314104– user3141042013年09月28日 23:37:41 +00:00Commented Sep 28, 2013 at 23:37
-
\$\begingroup\$ Related: stackoverflow.com/q/1741160/435605 \$\endgroup\$AlikElzin-kilaka– AlikElzin-kilaka2016年05月26日 16:01:02 +00:00Commented May 26, 2016 at 16:01
4 Answers 4
Your code does have its share of C-isms, but we will be able to fix that.
But first, if you are learning Java in order to understand Object Oriented Programming, pick a better language. Java's OO primitives are classes, abstract classes, interfaces and single inheritance. Since Java was designed, the world has moved on, and trait-based OO (instead of interfaces) is all the rage. The Scala language isn't very C-like, but has a wonderful object system. Another interesting language for OO is Ruby with it's metaclasses.
I assume you already know about the difference of static
and non-static fields and methods. Whatever is marked as static belongs to the class and is shared among all objects of that class. Your fillCharacters
static method only operates on the class level, yet you call it in an object constructor. Instead of refilling the characters
each time you make a new generator, initialize that array once.
A horrible C-ism is that you make your characters
array fixed size, in the hope that it will be large enough. But you only fill it with 63 characters! This is the way bugs are born. Java has multiple collections (like LinkedList
s or ArrayList
s) that resize themselves if necessary. I would do something like this:
private static char[] characters = initCharacters();
private static char[] initCharacters() {
final int initialCapacity = 63;
// a vector is a variable-size array
final List<Character> chars = new Vector<Character>(initialCapacity);
// add digits 0–9
for (char c = '0'; c <= '9'; c++) {
chars.add(c);
}
// add uppercase A–Z and '@'
for (char c = '@'; c <= 'Z'; c++) {
chars.add(c);
}
// add lowercase a–z
for (char c = 'a'; c <= 'z'; c++) {
chars.add(c);
}
// Copy the chars over to a simple array, now that we know
// the length. The .toArray method could have been used here,
// but its usage is a pain.
final char[] charArray = new char[chars.size()];
for (int i = 0; i < chars.size(); i++) {
charArray[i] = chars.get(i).charValue();
}
return charArray;
}
private static void showCharacters() {
System.err.println("The following " + characters.length + " characters are available: " + new String(characters));
}
What was that characters.length
? Yes, Java keeps book about the size of your arrays, so you don't have to! Keeping extra variables around for the size is error prone. Also note that I allocate various objects in the initCharacters
method – the garbage collector will clean up after me.
Because our character
array has no unused fields, we don't need the charactersSize
and charactersCount
variables any longer.
Something that C doesn't have is generics, i.e. parameterized polymorphism. The arguments in angled brackets are type names, which can only be classes. Java has primitive types like char
and int
. These have corresponding object types like Character
and Integer
.
Your get
method is mostly allright. I'd call it nextPassword
, and would rename rnd
to random
. The algorithm is fine.
One thing I'd do differently would be not using much object orientation for this problem. Creating a new PasswordGenerator
for different sizes is silly. I'd rather use that functionality like
System.out.println(PasswordGenerator.nextPassword(passwordSize));
i.e. use the generation as a static method, and pass the requested size as a parameter.
-
3\$\begingroup\$ +1, nice answer. But have to disagree about Java being a bad language to learn object-oriented principles. I personally think that Scala "feels" very dirty and messy, but of course that's subjective. \$\endgroup\$asteri– asteri2013年09月28日 17:19:39 +00:00Commented Sep 28, 2013 at 17:19
-
2\$\begingroup\$ Is there any reason you chose to use Vector rather than ArrayList? This topic would suggest you should avoid its use: stackoverflow.com/questions/1386275/… \$\endgroup\$Bort– Bort2013年10月04日 21:58:47 +00:00Commented Oct 4, 2013 at 21:58
Here's my take on this. This is actually port of my C# code which does the same, however, as Java doesn't support functional programming aspect of C#, it's a bit less verbose.
I'm using really simple Fluent interface here.
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;
public class PasswordBuilder {
private final static Random random = new Random();
// we keep our data in lists. Arrays would suffice as data never changes though.
private final static List<Character> LOWER_CAPS, UPPER_CAPS, DIGITS, SPECIALS;
// stores all templates
private List<Template> templateList = new ArrayList<Template>();
// indicates if we should shuffle the password
private boolean doShuffle;
/**
* Factory method to create our builder.
*
* @return New PasswordBuilder instance.
*/
public static PasswordBuilder builder() {
return new PasswordBuilder();
}
/**
* Adds lowercase letters to password.
*
* @param count Number of lowercase letters to add.
* @return This instance.
*/
public PasswordBuilder lowercase(int count) {
templateList.add(new Template(LOWER_CAPS, count));
return this;
}
public PasswordBuilder uppercase(int count) {
templateList.add(new Template(UPPER_CAPS, count));
return this;
}
public PasswordBuilder digits(int count) {
templateList.add(new Template(DIGITS, count));
return this;
}
public PasswordBuilder specials(int count) {
templateList.add(new Template(SPECIALS, count));
return this;
}
/**
* Indicates that the password will be shuffled once
* it's been generated.
*
* @return This instance.
*/
public PasswordBuilder shuffle() {
doShuffle = true;
return this;
}
/**
* Builds the password.
*
* @return The password.
*/
public String build() {
// we'll use StringBuilder
StringBuilder passwordBuilder = new StringBuilder();
List<Character> characters = new ArrayList<Character>();
// we want just one list containing all the characters
for (Template template : templateList) {
characters.addAll(template.take());
}
// shuffle it if user wanted that
if (doShuffle)
Collections.shuffle(characters);
// can't append List<Character> or Character[], so
// we do it one at the time
for (char chr : characters) {
passwordBuilder.append(chr);
}
return passwordBuilder.toString();
}
// initialize statics
static {
LOWER_CAPS = new ArrayList<Character>(26);
UPPER_CAPS = new ArrayList<Character>(26);
for (int i = 0; i < 26; i++) {
LOWER_CAPS.add((char) (i + 'a'));
UPPER_CAPS.add((char) (i + 'A'));
}
DIGITS = new ArrayList<Character>(10);
for (int i = 0; i < 10; i++) {
DIGITS.add((char) (i + '0'));
}
// add special characters. Note than other
// than @, these are in ASCII range 33-43
// so we could have used the loop as well
SPECIALS = new ArrayList<Character>() {{
add('!');
add('@');
add('#');
add('$');
add('%');
add('^');
add('&');
add('(');
add(')');
add('*');
add('+');
}};
}
}
Here's the Template class. This class is used to tell us something like: From list of given characters, take n(count) characters by random.
import java.util.ArrayList;
import java.util.List;
import java.util.Random;
public class Template {
private final List<Character> source;
private final int count;
private static final Random random = new Random();
public Template(List<Character> source, int count) {
this.source = source;
this.count = count;
}
public List<Character> take() {
List<Character> taken = new ArrayList<Character>(count);
for (int i = 0; i < count; i++) {
taken.add(source.get(random.nextInt(source.size())));
}
return taken;
}
}
Last, but not the least, example usage:
PasswordBuilder builder = new PasswordBuilder();
builder.lowercase(5)
.uppercase(5)
.specials(2)
.digits(2)
.shuffle();
// write 100, 14-char shuffled passwords
for (int i = 0; i < 100; i++) {
System.out.println(builder.build());
}
This example (hopefully) shows the power of OO. It was not necessary for this to be this complex, you can remove templates and just generate password on the fly, or you can improve them and add more features.
In the end, there are tens of different ways to implement this, but I hope I helped a bit. :)
-
\$\begingroup\$ +1, and I am not going to select this as the accepted answer just because due to my poor experience in the world of OOP, I can't get through your code so well as I can read amon's. I really thank you for the example, though. Indeed, the more I read it, the more it makes sense to me, and I already prefer your approach to the one I initially took. Thank you @Alex. \$\endgroup\$doplumi– doplumi2013年10月06日 18:12:33 +00:00Commented Oct 6, 2013 at 18:12
-
1\$\begingroup\$ why have you implemented a Fisher-Yates algorithm when there is a Collections.shuffle(List<?> list, Random rnd), available? \$\endgroup\$doplumi– doplumi2013年10月06日 21:29:07 +00:00Commented Oct 6, 2013 at 21:29
-
\$\begingroup\$ check docs.oracle.com/javase/7/docs/api/java/util/… for that \$\endgroup\$doplumi– doplumi2013年10月06日 21:29:37 +00:00Commented Oct 6, 2013 at 21:29
-
\$\begingroup\$ You should by all means accept the other answer, as It's based on your original code. I have just provided a way how would I myself would have implemented it, and of course I'm not claiming it's any better - moreover, it has its downsides. You are correct about Collections.shuffle(), I'll change the code to reflect that (No built-in shuffle in .net, so that was the reason as I hastly ported it ). \$\endgroup\$Alex– Alex2013年10月07日 03:26:51 +00:00Commented Oct 7, 2013 at 3:26
Its not required to write tons of lines and regex for this purpose now. We can use passey library that generates and validates passwords. Passwords can be alphanumeric or special characters.
For example following methods generates password with alphanumeric and special characters with a minimum length of 8 characters.
public String generateRandomPassword() {
List rules = Arrays.asList(new CharacterRule(EnglishCharacterData.UpperCase, 1),
new CharacterRule(EnglishCharacterData.LowerCase, 1), new CharacterRule(EnglishCharacterData.Digit, 1),new CharacterRule(EnglishCharacterData.Special, 1));
PasswordGenerator generator = new PasswordGenerator();
String password = generator.generatePassword(8, rules);
return password;
}
Reference: Generating random Password in Java
-
1\$\begingroup\$ A bit more detail might be warranted, especially since this question has had an accepted answer for about three years. In any case, I don't think the author is still concerned about this code... \$\endgroup\$Graipher– Graipher2017年05月03日 08:19:19 +00:00Commented May 3, 2017 at 8:19
-
\$\begingroup\$ I have updated the answer with an example \$\endgroup\$Dhiraj Ray– Dhiraj Ray2017年05月03日 08:54:24 +00:00Commented May 3, 2017 at 8:54
-
1\$\begingroup\$ This may be helpful for others who are looking for the random password generator same as I was looking for it and found a better approach to generate it \$\endgroup\$Dhiraj Ray– Dhiraj Ray2017年05月03日 09:01:35 +00:00Commented May 3, 2017 at 9:01
-
1\$\begingroup\$ That looks better already. I'm not familiar with the code formatting conventions in Java, but that list of character rules might need some nicer formatting. In addition you should also specifically add the requirements for this to work (passey, if I understand that link correctly). \$\endgroup\$Graipher– Graipher2017年05月03日 09:11:15 +00:00Commented May 3, 2017 at 9:11
@doplumi, some feedback on you solution:
- It's not customizable. Hard to change the possible chars.
- You wrote the int value of the chars themselves, like 48. Hard to understand. Documentation can help but we should try to be more explicit in the code. Example: '0'.
- Many indexes in your loops. Hard to follow, especially if some error snuck in.
Implemented one myself as well. As a builder.
Supports password limit, must have characters and how much of them, char ranges and a list of them.
Usage Example:
System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).build());
Example result: QU1GY7p+j+-PUW+_
System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).setSize(5).build());
Example result: %,4NX
Implementation:
//Version=1.0
//Source=https://www.dropbox.com/s/3a4uyrd2kcqdo28/PasswordBuilder.java?dl=0
//Dependencies=java:7 com.google.guava:guava:18.0 commons-lang:commons-lang:2.6
import com.google.common.primitives.Chars;
import org.apache.commons.lang.ArrayUtils;
import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
/**
* Created by alik on 5/26/16.
*/
public class PasswordBuilder {
private int size = 16;
private List<Character> options = new ArrayList<>();
private Map<List<Character>, Integer> musts = new java.util.LinkedHashMap<>();
private SecureRandom secureRandom = new SecureRandom();
public PasswordBuilder() {
}
public PasswordBuilder setSize(int size) {
this.size = size;
return this;
}
public PasswordBuilder addRangeOption(char from, char to, int mustCount) {
List<Character> option = new ArrayList<>(to - from + 1);
for (char i = from; i < to; ++i) {
option.add(i);
}
return addOption(option, mustCount);
}
public PasswordBuilder addCharsOption(String chars, int mustCount) {
return addOption(Chars.asList(chars.toCharArray()), mustCount);
}
public PasswordBuilder addOption(List<Character> option, int mustCount) {
this.options.addAll(option);
musts.put(option, mustCount);
return this;
}
public String build() {
validateMustsNotOverflowsSize();
Character[] password = new Character[size];
// Generate random from musts
for (Map.Entry<List<Character>, Integer> entry : musts.entrySet()) {
for (int i = 0; i < entry.getValue(); i++) {
int charIndex = secureRandom.nextInt(entry.getKey().size());
char c = entry.getKey().get(charIndex);
addChar(password, c);
}
}
// Generate from overall
for (int i = 0; i < password.length; i++) {
if (password[i] != null) continue;
password[i] = options.get(secureRandom.nextInt(options.size()));
}
return new String(ArrayUtils.toPrimitive(password));
}
private void addChar(Character[] password, char c) {
int i;
for (i = secureRandom.nextInt(password.length); password[i] != null; i = secureRandom.nextInt(password.length)) {
}
password[i] = c;
}
private void validateMustsNotOverflowsSize() {
int overallMusts = 0;
for (Integer mustCount : musts.values()) {
overallMusts += mustCount;
}
if (overallMusts > size) {
throw new RuntimeException("Overall musts exceeds the requested size of the password.");
}
}
public static void main(String[] args) {
System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).build());
System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).setSize(5).build());
}
}
Explore related questions
See similar questions with these tags.