3
\$\begingroup\$

I have made a ID generator class for my Repair Shop Software, that generates new ID for all data entities that are used in the software. It also converts int value of the ID to String value used for UI, and String values used for UI, to int value. I would like a general opinion on my IDGenerator class. Am I using some anti-pattern, and is my code clear enough. My project is on the GitHub.

IDGenerator.java

public class IDGenerator
{
    private static final int WORKSTATION_ID = 1;
    private static final int DAY_ID_FORMATER = 100;
    private static final int MONTH_ID_FORMATER = 10000; 
    private static final int YEAR_ID_FORMATER = 1000000;
    private static final int WORKSTATION_ID_FORMATER = 100000000;
    private static final int WORKSTATION_ID_VALUE = WORKSTATION_ID * WORKSTATION_ID_FORMATER;

    private static LocalDate lastTicketDate = LocalDate.MIN;

    public static int getNewID(EntityType entityType)
    {
        Objects.requireNonNull(entityType, "EntityType is null");

        int entityCounter = DataManager.getEntityCounter(entityType);

        switch(entityType)
        {
            case TICKET:
            {
                checkNewWorkDay();

                LocalDate today = LocalDate.now();

                int dayIDValue = today.getDayOfMonth() * DAY_ID_FORMATER;
                int monthIDValue = today.getMonthValue() * MONTH_ID_FORMATER;
                int yearIDValue = (today.getYear() % DAY_ID_FORMATER) * YEAR_ID_FORMATER;

                return entityCounter + 1
                     + dayIDValue
                     + monthIDValue
                     + yearIDValue
                     + WORKSTATION_ID_VALUE;
            }
            default:
            {
                return entityCounter + 1 + WORKSTATION_ID_VALUE;
            }
        }
    }

    private static void checkNewWorkDay()
    {
        if(lastTicketDate != LocalDate.now())
        {
            DataManager.resetTicketCounter();
            lastTicketDate = LocalDate.now();
        }
    }

    public static String toString(EntityType entityType, int id)
    {
        Objects.requireNonNull(entityType, "EntityType is null");

        String workstationID = String.valueOf(id / WORKSTATION_ID_FORMATER);

        switch(entityType)
        {
            case TICKET:
            {
                String date = String.valueOf((id % WORKSTATION_ID_FORMATER) / DAY_ID_FORMATER);
                String dailyTicketCounter = String.valueOf(id %  DAY_ID_FORMATER);

                if(dailyTicketCounter.length() == 1)
                {
                    dailyTicketCounter = '0' + dailyTicketCounter;
                }

                return workstationID + "-" + date + "-" + dailyTicketCounter;
            }

            default:
            {
                String entityCounter = String.valueOf(id % WORKSTATION_ID_FORMATER);

                return workstationID + "-" + entityCounter;
            }   
        }
    }

    public static int toInt(EntityType entityType, String displayName)
    {
        Objects.requireNonNull(entityType, "EntityType is null");

        if(LabelName.NULL_ITEM.equals(displayName))
            return 0;

        int workstationID = Integer.parseInt(displayName.split("-")[0]) * WORKSTATION_ID_FORMATER;

        switch(entityType)
        {
            case TICKET:
            {
                int date = Integer.parseInt(displayName.split("-")[1]) * DAY_ID_FORMATER;
                int entityCounterNumber = Integer.parseInt(displayName.split("-")[2]);
                return workstationID + date + entityCounterNumber;
            }
            default:
            {
                int entityCounterNumber = Integer.parseInt(displayName.split("-")[1]);

                return workstationID + entityCounterNumber;
            }
        }   
    }
}

IDGenertatorTest.java

public class IDGeneratorTest
{
    @ParameterizedTest
    @EnumSource(EntityType.class)
    public void getNewIDTest(EntityType entityType)
    {
        switch(entityType)
        {
            case TICKET:
                assertEquals(119123001, IDGenerator.getNewID(entityType));
                break;

            default:
                assertEquals(100000001, IDGenerator.getNewID(entityType));
                break;
        }
    }

    @ParameterizedTest
    @EnumSource(EntityType.class)
    public void toStringTest(EntityType entityType)
    {
        switch(entityType)
        {
            case TICKET:
                assertEquals("1-191230-01", IDGenerator.toString(entityType, 119123001));
                break;

            default:
                assertEquals("1-21", IDGenerator.toString(entityType, 100000021));
                break;
        }
    }

    @ParameterizedTest
    @EnumSource(EntityType.class)
    public void toIntTest(EntityType entityType)
    {
        switch(entityType)
        {
            case TICKET:
                assertEquals(119123001, IDGenerator.toInt(entityType, "1-191230-01"));
                break;

            default:
                assertEquals(100000021, IDGenerator.toInt(entityType, "1-21"));
                break;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

First of all we should describe a user-story for your ID-generator. This description should contain use-cases (including examples), requirements, and related technical context.

Use-Story

As creator of a new entity (system) I would like to make sure that this entity can be identified within the system by an Identifier (ID). So the ID should be automatically generated when the new entity is created and persisted.

As administrator or user of the system I would like to read some information about the entity directly from the generated ID. So the ID should be a Natural Key which encodes some business information.

Requirements:

  1. the ID should be numerical
  2. depending on the entity-type the ID could vary in shape/pattern
  3. the pattern and information encoded into the ID should tell something about it. Thus it contains informational parts, each encoded as a number. Parts may be workstation-id, running-counter or creation-date
  4. the informational parts (numbers) of the ID are separated by a dash -
  5. the running-counter should be (at least) unique per entity-type
  6. the default ID (if not specified separately for the entity-types) should contain two parts: (a) workstation-id as 1-digit, (b) running counter as 2-digits. So the pattern <workstation_id>-<count> can for example be represented as 1-01 for the first entity XY (running counter 01) created on workstation 1.
  7. the ID of an entity-type TICKET should consist of one extra information: (c) creation-date as 6-digit ISO-date in format yyMMdd. So the pattern <workstation_id>-<date_yyMMdd>-<count> can for example be represented as 1-191230-01 for the first ticket (running counter 01) created on workstation 1 on 30th of December 2019.
  8. additionally the running-counter of entity-type TICKET should be reset on each new creation-date. So that the running-counter is unique within each creation-date.

Design & Modelling

Now you can model the data structures needed to fulfill the requirements, i.e. classes or interfaces.

class EntityID: learn from Java's UUID class and its implemented interfaces:

public class EntityID implements Serializable, Comparable<EntityID> {
  public static final String SEPARATOR = "-";
  public static final int REQUIRED_PARTS = 2;

  protected int workstationId;
  protected int counter;

  public EntityID(int workstationId, int counter) {
    // could add some parameter validation (like only positive values)
    this.workstationId = workstationId;
    this.counter = counter;
  }

  @Override // implements Comparable<EntityID> 
  public int compareTo(EntityID value) {
    // needed to make sure if compared objects are same, or for sorting: lesser, greater
    // left for you to implement
    return -1;
  }

  public static EntityID fromString(String name) {
    if (name == null || name.trim().isEmpty()) {
      throw new IllegalArumentException("ID to parse must not be null or empty!");
    }
    String[] parts = name.split(SEPARATOR);
    if (parts.length != REQUIRED_PARTS) {
      throw new IllegalArumentException(String.format("ID to parse must consist of %d parts, separated by '%s', but was: %s", REQUIRED_PARTS, SEPARATOR, name));
    }
    int workstationId = Integer.parseInt(parts[0]);
    int counter = Integer.parseInt(parts[1]);
    return new EntityID(workstationId, counter);
  }

  // getter and setter for all information parts
  // left to implement

  public String toString() {
    return String.format("%01d%s%02d", workstationId, SEPARATOR, counter); 
  }
}

This class servers the default ID. For the special case TICKET you need to inherit this class, as follows.

class TicketId:

public class TicketID extends EntityID implements Serializable, Comparable<TicketID> {
  public static final String SEPARATOR = "-";
  public static final int REQUIRED_PARTS = 3;
  public static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyMMdd");

  protected LocalDate creationDateUtc;

  public TicketID(int workstationId, int counter, LocalDate utcDate) {
    super(workstationId, counter);
    this.creationDateUtc = utcDate;
  }

  @Override // implements Comparable<TicketID> 
  public int compareTo(TicketID value) {
    // needed to make sure if compared objects are same, or for sorting: lesser, greater
    // left for you to implement
    return -1;
  }

  public static TicketID fromString(String name) {
    if (name == null || name.trim().isEmpty()) {
      throw IllegalArumentException("ID to parse must not be null or empty!");
    }
    String[] parts = name.split(SEPARATOR);
    if (parts.length != REQUIRED_PARTS) {
      throw new IllegalArumentException(String.format("ID to parse must consist of %d parts, separated by '%s', but was: %s", REQUIRED_PARTS, SEPARATOR, name));
    }
    int workstationId = Integer.parseInt(parts[0]);
    LocalDate utcDate = LocalDate.parse(parts[1], DATE_FORMATTER);
    int counter = Integer.parseInt(parts[2]);
    return new TicketID(workstationId, counter, utcDate);
  }

  // getter and setter for all information parts
  // left to implement

  public String toString() {
    // could also use a DateFormatter like
    // return String.format("%01d-%s-%02d", workstationId, DATE_FORMATTER.format(creationDateUtc), counter);
    return String.format("%01d-%tY%tm%td-%02d", workstationId, creationDateUtc, counter);
  }
}

Note:

How parsing (form representation String to value ID) and formatting (from value ID to representation String) was implemented using static facotry method fromString and instance method toString.

The above classes implement requirements 1. - 4. and 6. - 7.

Now the ID-generator comes into play. Using Factory Design-Pattern we can implement it like follows.

The counter for each entity-type should be managed inside the generator, not be dependent on some DataManager. Thus make use of Java's concurrent counter classes [AtomicInteger](https://stackoverflow.com/questions/4818699/practical-uses-for-atomicinteger]

public class EntityIdGenerator {

   protected AtomicInteger counter;
   protected Integer counterMax;
   protected Integer workstationId; 

   // here the current value from DataManager can be injected as counterStart
   // counterMax = 99 (limit to 2-digits), workstationId = 1
   public EntityIdGenerator(int counterStart, int counterMax, int workstationId) {
     if (counterMax <= counterStart || counterStart < 0 || counterMax < 1) {
       throw new IllegalArgumentException("CounterStart must positive and >= 0, counterMax must be > 1 and > counterStart"!); 
     }
     this.counter = new AtomicInteger(counterStart);
     this counterMax = counterMax;
     this.workstationId = workstationId;
   }

   protected int rollToNextCounter() {
     // if current counter is 99 == counterMax, then reset  
     boolean hasReachedMaxAndReset = this.counter.compareAndSet(counterMax, 0);
     // if counterStart was 0, the nextCounter will also be 0 (first used) 
     return this.counter.getAndIncrement();     
   }

   public EntityID generateId() {
     return new EntityID(this.workstationId, rollToNextCounter());
   }

}

The above class has some benefits:

  • can now be tested isolated without need of some DataManager (independent)
  • can now have different instances (each of it manages state for a special entity-type, and its counter).

With the last (multiple instances) we implemented requirement 5.

  • can be specialized, meaning it can be inherited to some class TicketIdGenerator which manages some property lastIssuedDateUtc in order to reset the counter even if the counterMax has not yet reached.
public class TicketIdGenerator extends EntityIdGenerator {
   private LocalDate lastIssuedDateUtc;

   public TicketIdGenerator(int counterStart, int counterMax, LocalDate lastIssuedDateUtc) {
     super(counterStart, counterMax);
     if (lastIssuedDateUtc > todayUtc()) {
       throw new IllegalArgumentException("date (as UTC) must can be now or in the past; must not be in the future: " + lastIssuedDateUtc);
     }
     this.lastIssuedDateUtc = lastIssuedDateUtc;
   }

   public LocalDate todayUtc() {
     return LocalDate.now("UTC");
   }

   public TicketID generateId() {
     if (this.lastIssuedDateUtc < todayUtc()) {
       this.lastIssuedDateUtc = todayUtc();
     }
     return new TicketID(this.workstationId, rollToNextCounter(), this.lastIssuedDateUtc);
   }

   // rest of logic and methods remain same as in EntityIdGenerator

}

With above we implemented requirement 8.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ This is not a code review. You are suggesting your own solution without considering the code in question. You also come up with your own requirements disregarding requirements that can be deriven from question/code in question \$\endgroup\$
    – purple
    Jan 2 '20 at 23:43
  • 1
    \$\begingroup\$ Above code is not yet tested plus there are at least 2 points not yet designed: (a) TicketID represented by non-UTC date (in local timezone); (b) assignment of each ID-generator instance to respective EntityType. If you see some mistakes (e.g. one method should be static), comment ;-) \$\endgroup\$
    – hc_dev
    Jan 2 '20 at 23:44
  • \$\begingroup\$ @purple Yes it's my proposal of an improved design (not one monolithic class, violating SRP, etc.). I will explain and provide traditional review-comments to OP's source tomorrow. And please list the requirements that you can derive from OP's post !? \$\endgroup\$
    – hc_dev
    Jan 2 '20 at 23:50
  • 1
    \$\begingroup\$ I appreciate your extensive answer, and I intend to implement some of your suggestions. Test isolation is one of my concerns, and I was thinking of re-implementing IDGenerator just because of that. \$\endgroup\$ Jan 3 '20 at 13:54
3
\$\begingroup\$

Using static methods only for your IDGenerator class is antipattern. It's better to make those instance methods and create instance of your class when you need it. One reason may be extendability, another can be thread safety. I have a feeling, that static local variable lastTicketDate will probably cause trouble with race conditions.

If you really want to have static access to it, consider making it singleton with static method to get the instance. It's not ideal, but still better approach and it's on the way to making your code non-static, keeping reference to your class in local/class variable rather than static access. Thread safety can still be issue since any thread can access statically your class as long as there is static method to get it.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ You may have some misconceptions about singletons. You don't have static access to a singleton since a singleton is an object (that guarantees that there is only one instance). Singletons are also usually an anti pattern. You should rely on dependency injection in order to guarantee against duplicate instantiations. Static access has nothing to do with guaranteeing thread safety. Same restrictions apply as with instance methods. \$\endgroup\$ Dec 31 '19 at 15:24
  • 1
    \$\begingroup\$ True it was a bit vague (edited a bit). I meant access to singleton using static factory. I never said that singleton is best way, I suggested it as mid-way to better code that doesn't require full refactor. I also never said that static access guarantees thread safety, quite opposite - I mentioned, that it can still cause issue. If you wanna talk this through or chat or something, I am happy to discuss. \$\endgroup\$
    – K.H.
    Jan 3 '20 at 7:39
  • \$\begingroup\$ Can you explain me what bad could potentially happen in the program with my solution regarding thread safety. \$\endgroup\$ Jan 3 '20 at 12:54
1
\$\begingroup\$

Using plain addition is going to create identical ID's. For instance, if for 1 ID the dayIDValue is 6 and the monthIDValue is 5, you'll get the same ID if next time the dayIDValue is 5 and the monthIDValue is 6.

I would suggest using the UUID class. It allows you to choose different versions depending on how unique you need the ID's to be.

\$\endgroup\$
1
  • \$\begingroup\$ Yes, I have consider using UUID numbers for ID's, but they are very large considering I plan to have entire database in RAM. I don't think you have read the code of my class, because the possible duplicate ID is only possible if the daily ticket counter is larger then 99, but that would be prohibited in the client. If you see the test class it will be clear. The fifth ticket on the workstation with the workstation ID of 2 on 31.12.2019. will have 219123105 ID. My ID also caries some information unlike UUID. Thank you for your suggestion. \$\endgroup\$ Dec 31 '19 at 7:29

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.