9
\$\begingroup\$

I'm learning about immutability and uniqueness in Java, particularly the use of defensive programming and object factories.

I have been asked to create a class containing an object factory that when called creates a unique registration number comprised of a single letter and 4 digits. I think my solution ticks all boxes, but would appreciate some input on this one before I go any further to see if I'm on the right track.

public class RegistrationNumber
{
    private static final Map<String, RegistrationNumber> REG = new HashMap<String, RegistrationNumber>();  

    private static int number;
    private static char letter;
    private static String strRep;

    private RegistrationNumber(int number, char letter, String strRep) {

      this.number = number; 
      this.letter = letter;
      this.strRep = strRep;

    }      


    public static final RegistrationNumber getInstance() {

       Random r = new Random();

       int numbers = r.nextInt(9000) + 1000;

       Character letter = (char)(r.nextInt(26) + 'a');

       String strRep = letter + "" + numbers;

       RegistrationNumber n = REG.get(strRep);

       if (n == null) {

           n = new RegistrationNumber(numbers, letter, strRep);

           REG.put(strRep, n);

        }
             return n;
           }


      char getLetter() {

          return letter;

        }

      int getNumbers() {

          return number;

        }


      public String toString() {

       return strRep;

    }

}
\$\endgroup\$
11
\$\begingroup\$

The number, letter, and strRep fields should be final and must not be static. By making them static, you've made all of your RegistrationNumber objects very weird, and quite the opposite of immutable. Constructing one RegistrationNumber changes every previously generated RegistrationNumber!

getInstance() is a bit odd. Conventionally, the name getInstance() would retrieve a singleton instance — it should the same object every time it is called. What you have instead is a method that usually generates a new object, but occasionally returns an existing object. I'm puzzled by the motivation behind that behaviour.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ By convention, getInstance() returns an instance, not a singleton. It's used to allow the class flexibility in whether or not the instance returned is the same every time or not. \$\endgroup\$ – Eric Stein Mar 6 '15 at 14:54
9
\$\begingroup\$

Let's look at the requirements, an immutable unique Registration number which consists of a letter and 4 digits, constucted by a factory method. There are 4 parts here:

  1. immuatable
  2. letter/number combination
  3. unique
  4. factory.

Immutable

This is easy to solve, but harder to describe. A Java class that is final, has only private and final fields, is Immutable. In our case:

public final RegistrationNumber {
    private final String number;

    public RegistrationNumber(String number) {
        this.number = number;
    }

    public String getNumber() {
        return number;
    }
}

Letter/number

The letter/number combination makes things interesting. You have chosen to use random values. There is nothing in the spec about that. I think it would be simpler to use sequential values. Also, 0 is a number, what's wrong with A0001 as a value?

Here's a system that will produce a letter/number combination:

int currentNumber = 0;
int currentChar = 'A';

return String.format("%s%04d", currentChar, currentNumber);

That will 0-pad numbers less than 1000 to make them 4-digits wide.

Unique and factory

The factory is what makes the unique number, so you have to describe them together.

Also, a factory method means the constructor for the RegistrationNumber should be private.

A factory method would look like:

private static int count = 0;

public Registration newInstance() {
    count++;
    int val = count % 10000;
    char letter = (char)('a' + (count / 10000));
    return new RegistrationNumber(String.format("%s%04d", letter, val));
}

You may want to consider a thread-safe alternative, using an AtomicInteger instead of a plain int.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Your bit on immutability is incorrect. If I have a class that has a mutable member, and I expose that mutable member, then the class is not immutable. \$\endgroup\$ – Eric Stein Mar 6 '15 at 14:52
  • \$\begingroup\$ @EricStein - it's not incorrect, the class is fully immutable, and nothing in the class can be changed. If you have a different class, and reference an instance of that other class, and that other class is mutable, then that's a different problem. Your class is immutable, the other class is not. If you want an immutable system, then all instances referenced by you, and all subsequent references, all have to be immutable. I stand by what I said: A Java class that is final, has only private and final fields, is Immutable. \$\endgroup\$ – rolfl Mar 6 '15 at 15:31
  • 1
    \$\begingroup\$ I feel pretty good about my statement knowing Josh Bloch agrees with me. :-) "If your class has any fields that refer to mutable objects, ensure that clients of the class cannot obtain references to those objects. Never [..] return the object reference from an accessor." Effective Java 2.0, 15: Minimize Mutability. Part of the state of a class is the state of its members. How can the class be immutable if its state can change? \$\endgroup\$ – Eric Stein Mar 6 '15 at 16:48
  • \$\begingroup\$ Meh, I return a reference to a private member, yes, a String. My class is immutable, and I trust String's immutability. I don't expose any mutable content. You're being pedantic about something I'm not doing, unless you suggest I should be taking a defensive copy of the string before returning it? \$\endgroup\$ – rolfl Mar 6 '15 at 19:22
  • \$\begingroup\$ No, that's never what I said. I never said your class was mutable. I said your statement about what it meant to be immutable was in error. \$\endgroup\$ – Eric Stein Mar 6 '15 at 19:55
4
\$\begingroup\$

Adding to the previous good reviews which left not much out.

If you use a Random, you should ensure that you reuse a created instance. This is because if you create a random very shortly after another ( for Java 8: < 1ns which is a long time in computing) the Random will be created using the same seed it can happen that you will get the same "random" value.

private final static Random random = new Random();
public static final RegistrationNumber getInstance() {

   int numbers = random.nextInt(9000) + 1000;

   Character letter = (char)(random.nextInt(26) + 'a')
   ...  
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ FYI Java6 attempted (badly) to solve that problem, and Java7 got it right. \$\endgroup\$ – rolfl Mar 6 '15 at 10:56
1
\$\begingroup\$

In addition to what has already been mentioned, the string representation of the name is a derived property of the registration number and as such I believe that it should be computed internally in the constructor. This saves you one argument to the constructor and avoids code duplication if you have multiple factories.

Like this:

private RegistrationNumber(int number, char letter) {
  this.number = number; 
  this.letter = letter;
  this.strRep = String.format("%s%04d", letter, number);
}      
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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