6 Replies - 484 Views - Last Post: 06 October 2013 - 04:28 AM Rate Topic: -----

#1 domenico  Icon User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 89
  • Joined: 21-July 12

Critiques on my newbie Java class code

Posted 28 September 2013 - 01:36 AM

Hi guys,
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, 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() );   
    }
  
}

This post has been edited by domenico: 28 September 2013 - 02:46 AM


Is This A Good Question/Topic? 0
  • +

Replies To: Critiques on my newbie Java class code

#2 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 362
  • View blog
  • Posts: 1,534
  • Joined: 08-April 09

Re: Critiques on my newbie Java class code

Posted 28 September 2013 - 02:37 AM

The fillCharacter method can be void, you just fill your array, i don't see what else you are doing with your characters. And charactersSize can be 63 not 100, a-z is 26, A-Z is also 26, 0-9 is 10 and @ is 1 in total is 63.

And also for this
 
      // choose a random character from the array
      for ( int i = 0; i < passwordSize; ++i ) {
        password[i] = characters[ rnd.nextInt(charactersCount) ];
      }


I would suggests to read about Fisher -Yates algorithm

This post has been edited by Michael26: 28 September 2013 - 02:43 AM

Was This Post Helpful? 2
  • +
  • -

#3 domenico  Icon User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 89
  • Joined: 21-July 12

Re: Critiques on my newbie Java class code

Posted 28 September 2013 - 02:45 AM

Quote

The fillCharacter method can be void, you just fill your array, i don't see what else you are doing with your characters.

I just wanted to separate concerns. If I would like to change the characters in the array at a later time, I would know where to go.
It doesn't make much difference now but when the constructor starts to grow bigger, don't you thing it would be better to have all this functionality organized?
What's your point, why should I fill the array in the constructor? Performance?
Maybe I should rename it initCharacters though... In fact, I'll do it just now.

This post has been edited by domenico: 28 September 2013 - 02:46 AM

Was This Post Helpful? 0
  • +
  • -

#4 domenico  Icon User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 89
  • Joined: 21-July 12

Re: Critiques on my newbie Java class code

Posted 28 September 2013 - 02:50 AM

Quote

And also for this
 
      // choose a random character from the array
      for ( int i = 0; i < passwordSize; ++i ) {
        password[i] = characters[ rnd.nextInt(charactersCount) ];
      }


I would suggests to read about Fisher -Yates algorithm


Nice catch! thank you for that :)
Was This Post Helpful? 0
  • +
  • -

#5 Michael26  Icon User is offline

  • DIC-head, major DIC-head
  • member icon

Reputation: 362
  • View blog
  • Posts: 1,534
  • Joined: 08-April 09

Re: Critiques on my newbie Java class code

Posted 28 September 2013 - 03:30 AM

I modified this a bit, i have put the Random instance as class variable, you don't need to create new Random instance everytime you call get method
// generate a random password
    public char [] get() {

        // initialize the random number generator
        char [] password = new char [passwordSize];
        // Shuffle and choose a random character from the array
        ShuffleArray(characters);
        for ( int i = 0; i < passwordSize; ++i ) {
            password[i] = characters[i]; //ShuffleArray method shuffles your array and then you can just print first [il]passwordSize[/il] characters, you can also leave it the way you did in original.
        }
        return password;
    }


private void ShuffleArray(char[] characters) {
                 for (int i = characters.length -1; i >0; i--)
                 {
                     int index = rnd.nextInt(i);
                     char temp = characters[index];
                     characters[index] = characters[i];
                     characters[i] = temp;
                 }
    }

This post has been edited by Michael26: 28 September 2013 - 03:32 AM

Was This Post Helpful? 1
  • +
  • -

#6 baavgai  Icon User is offline

  • Dreaming Coder
  • member icon

Reputation: 5882
  • View blog
  • Posts: 12,760
  • Joined: 16-October 07

Re: Critiques on my newbie Java class code

Posted 28 September 2013 - 07:15 AM

Get rid of the static. e.g.
class PasswordGenerator {
	private final char [] characters ;
	private final Random rnd;
	public PasswordGenerator() { /* your code here */ }
	public String get(int passwordSize) { /* your code here */ }
}



Note, return type is String, which seems more useful.

Conversely, you could go all static:
class PasswordGenerator {
	private static final char [] characters = initCharacters();
	private static final Random rnd = new Random();
	private static char [] initCharacters() { /* your code here */  }
	public static String get(int passwordSize) { /* your code here */ }
}



If you think about it, you could probably do this:
class PasswordGenerator {
	private static final Random rnd = new Random();
	private static char randChar() { /* your code here */  }
	public static String get(int passwordSize) { /* your code here */ }
}



Indeed, you might want to implement a randChar in any case, just to abstract it a little more.
Was This Post Helpful? 1
  • +
  • -

#7 domenico  Icon User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 89
  • Joined: 21-July 12

Re: Critiques on my newbie Java class code

Posted 06 October 2013 - 04:28 AM

Figured it out! Check below for more info!

Taking into account what you have suggested, I'm trying to rewrite the class from scratch :P/>/>
I've come to something I can't resolve though, I'll post here the relevant part of the code
public final class CustomString {
    // static list of chars with which the password can be built
    private final static List<Character> LOWER_CAPS, UPPER_CAPS;
    
    // some other code here...

    // initialize the statics
    private final static int ALPHABET_SIZE = 26;
    static {
        LOWER_CAPS = new ArrayList<Character>(ALPHABET_SIZE);
        UPPER_CAPS = new ArrayList<Character>(ALPHABET_SIZE);
        for ( char i = 'a'; i < ALPHABET_SIZE; ++i ) {
          LOWER_CAPS.add(i);
        }
        for ( char i = 'A'; i < ALPHABET_SIZE; ++i ) {
          UPPER_CAPS.add(i);
        }
    }

    public static void main(String[] args) {
        CustomString customString = new CustomString();
        System.out.println(CustomString.UPPER_CAPS.size());
    }
}



When I launch the class from the terminal, it will always be printed 0. If I insert a System.out.println("something..") statement inside the static{..} block though, the code is executed and "something.." is printed out, so it seems like the initialization is correctly executed.
Also, if I try to print CustomString.UPPER_CAPS.size() just after the last for cycle, it will print out 0!
I can't continue on just because of this...
What is going on?

Shame on me :/ It was a silly non-Java mistake.
Spoiler

This post has been edited by domenico: 06 October 2013 - 02:51 PM

Was This Post Helpful? 0
  • +
  • -

Page 1 of 1