2
\$\begingroup\$

I have a abstract class which is extended by many many other classes:

What I have done:

public abstract class AbstractActionHandler {

    protected WorkItem currentWI;
    protected String status;
    protected String field;

    protected void init(WorkItem currentWI, String status, String field) throws IllegalArgumentException, SecurityException, IllegalAccessException, InvocationTargetException, NoSuchMethodException {
      this.currentWI = currentWI;
      this.status = status;
      this.field = field;
      if(couldBeExecuted()) {
        this.executeAction();
      }
    }

    protected boolean couldBeExecuted() throws IllegalArgumentException, IllegalAccessException, InvocationTargetException, SecurityException, NoSuchMethodException {
      long start = System.currentTimeMillis();
      DataObjectFactory fact = DataObjectFactory.getFactory(currentWI);
      DataWorkItem currentWI = fact.getDataWorkItem("current");
      long end = System.currentTimeMillis();
      System.out.println("+++++ " + " Workitem Factory takes " + (end-start) + "ms " + "+++++");

      // Other logic which returns false or true
      // [...]
    }
    // Implementation of business logic
    protected abstract void executeAction();
    // Should call init function
    public void execute(WorkItem currentWI, String status, String field) throws IllegalArgumentException {
        this.init(currentWI, status, field);
    }

}

Now I figured out that the creation of DataWorkItem object takes a lot of time. Because I am using an abstract factory I can simply change the line

DataObjectFactory fact = DataObjectFactory.getFactory(currentWI);

to

DataObjectFactory fact = DataObjectFactory.getBoostFactory(currentWI);

and provide another implementation of DataWorkItem object.

Old implementation which calls an external API function which needs a lot of time and which is basically only necessary to call once:

public DataWorkItem(IWorkItem wi, String revision) {
       OtherExternService externService = (OtherExternService) PlatformContext
       .getPlatform().lookupService(OtherExternService.class);  
       IPObjectList listOfRevs = externService.getDataService().getObjectHistory(wi);
}

New implementation which calls API function only one time because of singleton implementation:

public DataWorkItem(WorkItem wi, String revision, boolean global) {
        // +++
        // We use a global object here which holds the history of current work item.
        // We only need to read this history once so we create a singleton here.
        // The performance is hardly improved by this pattern.
        // We use this constructor in DataBoostObjectFactory.
        // +++
        IPObjectList listOfRevs = WorkItemGlobal.getInstance(wi).getObjectList();           
}

Singleton class:

public class WorkItemGlobal {

    public static WorkItemGlobal instance = null;
    private IPObjectList listOfRevs;

    private WorkItemGlobal(WorkItem wi) {
       OtherExternService externService = (OtherExternService) PlatformContext
       .getPlatform().lookupService(OtherExternService.class);  
       listOfRevs = externService.getDataService().getObjectHistory(wi);
    }

    public static WorkItemGlobal getInstance(WorkItem wi) {
       if(instance == null) {
           instance = new WorkItemGlobal(wi);
       }
       return instance;
    }

    public static void deleteInstance() {
       instance = null;
    }

    public IPObjectList getObjectList() {
       return listOfRevs;
    }

}

Question:

Is it good to use a Singleton here?

Of course I can avoid using a singleton if I create the DataWorkItem object outside of the classes which extends AbstractActionHandler and pass the object to the constructor (for example).

But with this approach I have to adjust ALL classes which extends AbstractActionHandler because I have to call a function (or change constructor) which passes the DataWorkItem object to the abstract class.

At all it works good (for now). I can see a hardly improved performance because the call to the external API function is only done once.

Other suggestions or ideas?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your question is: Is it good to use a Singleton here?

My answer is: If you can avoid using a singleton, do it! And you answer this yourself: "Of course I can avoid using a singleton if (...)".

Passing a DataWorkItem object to the constructor or some other method is more preferable than using a singleton-pattern. This is part of the principle Tell, don't ask.

The deleteInstance method in your "singleton" defies the Singleton pattern, which states that it

restricts the Instantiation of a class to one object.

By using a deleteInstance method, you allow multiple objects. (Not at the same time, but still multiple objects).

Other comments

You have one field protected WorkItem currentWI; and one local variable in your couldBeExecuted method: DataWorkItem currentWI = fact.get... They share the same variable name. This leads to confusion for me. I don't see much of your field. (Of course, I don't see your subclasses to this method). You might want to pass the object on to the methods instead of storing it as a field (again with the principle **Tell, don't ask). Will that object ever be used again after init has been called? If not, remove it.

In your DataWorkItem, you declare a new local variable on the last line of the constructor

IPObjectList listOfRevs = externService.getDataService().getObjectHistory(wi);

This should give you a "unused variable" warning from the compiler. What happens to this variable once the method is finished? Nothing!

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your comments! You are right. One of the "currentWI" variable should be renamed -> done. The protected currentWI field is used by sub classes. After the last line "IPObjectList listOfRevs = externService.getDataService().getObjectHistory(wi);" in my question there is also other relevant code which uses "listOfRevs". I forgot to comment that other code follows. However, you answer my question to avoid the Singleton (because it is just possible...independing of the effort). \$\endgroup\$
    – sk2212
    Commented Nov 21, 2013 at 14:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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