I'm having difficulty implementing a static factory for a number generator

Question

I've read all of the cries for "don't use static" and I understand that it reduces OOP and screws up unit tests and can break code when multi-threading. However, I'm attempting none of those.

I'm building a Java singleton utility that contains a public static number generator for issuing ID's - numbers which cannot be duplicated. I chose singleton so it can keep track of the numbers issued throughout the life of the program w/o having to reconstruct a million and having to worry about someone not de-referencing multiple instances. The initialization works, though it doesn't ever increment beyond its first invocation.

I've tried it this way:

    public class SClass {
      public static final SClass SINGLETON = getInstance();
      ...

      public static final int GEN_ID = genID();
      private static int base = 999999;

      ...
      ...

      private static int genID() {
        SClass.SINGLETON.base += 1;
        return base
      }
    }

and I've tried it this way:

    public class SClass {
      public static final SClass SINGLETON = getInstance();
      ... 

      public static int GEN_ID = genID();
      private int base;
      ...

      private SClass () {
        ...
        this.base = 999999;
        ...
      }

      ...
      ...

      private int genID() {
        this.base += 1;
        return base;
      }
    }

Yes, I've tried it with 'final' removed from everything...

In each implementation I invoke either strictly statically or I use an instance (SClass s = SClass.SINGLETON; s.Gen_ID) with both the static and object implementations described abaove. I only get "1000000" with both any initial and any consecutive invocations (of either methodology). Would someone be willing to explain what's going on here?

I'm not asking whether or not I should implement static (there's tons of pages with that already) - I'm only asking how to make this work, simply. Though I'm open to more elegant solutions.. Thanks in advance!

Answer 1

You could try this

class SClass {
    static final AtomicInteger COUNTER = new AtomicInteger();
    final int id = 1000000 + COUNTER.getAndIncrement();
}

No need to use synchronized as AtomicInteger is thread safe.

Answer 2

try this

public class IdGenerator {


    public static IdGenerator generator = new IdGenerator();    
    private IdGenerator(){}
    private static int currentID=0;
    private int getNextID(){
        synchronized (this) {
            currentID=currentID+1;  
        }
        return currentID;
    }

    public static IdGenerator getInstance(){
        return generator;
    }

    public static void main(String[] args) {
        IdGenerator generator  = IdGenerator.getInstance();
        for(int i=0;i<10;i++)
            System.out.println(generator.getNextID());
    }

}

Answer 3

Thank you to Jon, Peter and Upog for helping me on my issue. I wanted to show the real code for what my original issue was, and then show code for what solved it in the hopes that others may benefit from this particular case.

My original problem was that I couldn't increment a static, unrepeatable counter:

/**
 * Generate numbers and increment
 */
public class BuggedGenerator {

  /************** Public Constants / Factory ***********/
  private static BuggedGenerator INSTANCE = null;   // to contain the single instance

  /**
   * The single instance of BuggedGenerator.
   */
  public static final BuggedGenerator READ_IN = getInstance();
  public static final int GEN_ID = genID();
  private static int base = 999999;

  /************ Singleton SetUp ************/

  /**
   * Utility Constructor.
   */
  private BuggedGenerator() {
    super();                       // unnessesary, but I always invoke super()
  }

  /**
   * Initialize the counter singleton
   */
   private static int genID() {
        BuggedGenerator.SINGLETON.base += 1;
        return base
      }

  /**
   * Determine whether BuggedGenerator already has an instance
   * and return that instance.
   */
  public static BuggedGenerator getInstance() {
    if (null == BuggedGenerator.INSTANCE) {
        BuggedGenerator.INSTANCE = new BuggedGenerator();
    }
    return BuggedGenerator.INSTANCE;
  } // end getInstance()
}

This is what I was getting from this implementation:

> BuggedGenerator.READ_IN.GEN_ID
> 1000000
> BuggedGenerator.READ_IN.GEN_ID
> 1000000
> BuggedGenerator b = BuggedGenerator.READ_IN
> b.GEN_ID
> 1000000

When prompted with assistance, I used the AtomicInteger class to replace the GEN_ID implementation as shown in Peter's example, but I recieved compile-time errors about static initializations. I decided that it was too much of a pain to go against OOP and implemented the AtomicInteger as a conventional singleton, being a property of the object. Per Jon's suggestion I've included the whole of the code instead of a snapshot. Feel free to use:

 /**
  * Copyright 2013, Phil Reason.    preason intisive com
  * Permission to copy, modify, resell and or freely distribute - provided an 
  * exact copy of this file is explicitly accompanied and unaltered alongside 
  * of any distribution of works using this file or any modified version of 
  * this file. 
  */

import java.util.concurrent.atomic.AtomicInteger;

/** 
 * This is a class to generate numbers for various purposes. 
 * @author          Phil Reason
 * @conceptionDate  9/6/13
 * @version         1.1
 * @revisionDate    9/8/13
 */
public class Generator {
  /************** Constants *********************/  
  /**
   * The single instance of Generator.
   */
  public static final Generator READ_IN = getInstance();
  private static Generator INSTANCE = null;   // to contain the single instance
  /******** Instance Vars: *******************/
  private AtomicInteger counter;              // construct an AtomicInteger
  private int iDRange;

  /************ Singleton SetUp ************/
  /**
   * non-public default constructor override.
   */
  private Generator() {
    super();                       // unnessesary, but I always invoke super()
    this.iDRange = 1000000;        // the starting number to range increments 
    this.counter = new AtomicInteger(); // the AtomicInteger instance
  } //END Generator()

  /**
   * Determine whether Generator already has an instance
   * and return that instance.
   */
  private static Generator getInstance() {
    if (null == Generator.INSTANCE) {           // upon first use...
        Generator.INSTANCE = new Generator();   // construct the single instance
    }
    return Generator.INSTANCE;                  // return ony that instance
  } // END Generator getInstance()

  /**
   * Generate non-repeating numbers. This can be useful for serializing when 
   * inherited serialization isn't useful or needed.
   * 
   * Return the current count generation then increment the AtomicInteger.
   * @ensure genID() >= 1000000  && genID() != genID() (never repeats a number)
   */
  public int genID () {
    return iDRange + counter.getAndIncrement(); // increments the sum of counter
  }  // END int genID()

}

The output from this implementation is exactly what I needed, as it works for the lifespan of the class' memory residency. For that property, I only had to forecast each increment for JUnit in between tests when setUp() gets rerun - which does not de-reference the static class reference from memory. For the package I was testing, this was actually to my benefit. Here's what I got from output on this latter implement:

> Generator.READ_IN.GEN_ID
> 1000000
> Generator.READ_IN.GEN_ID
> 1000001
> Generator b = Generator.READ_IN
> b.GEN_ID
> 1000002
... and so on ...

In this implementation the AtomicInteger is used as like in any other object with traditional method invocation, though as a singleton. Not only did it work for what I needed, but I was also able to avoid breaking OOP design. I will need more practice before I'm comfortable enough to committing to static factories. Thanks again to you three for taking your time to answer my question! ~phil