2

I have some doubts regarding how to do the following operation on a class that follow the Singleton model.

I have this original Singleton class:

public class ThreadsManager {

    // I can have only one instance:
    private final static ThreadsManager instance = new ThreadsManager();

    // Private constructor:
    private ThreadsManager() {

    }

    public static ThreadsManager getInstance(){
        return instance;
    }
}

Ok, this work well but now I have to modify it adding a new property (a String named userName) that have to be initialized when the singleton object is build and that can not be changed at a later time

So I am trying to do something like it:

public class ThreadsManager {

    private final static ThreadsManager instance = new ThreadsManager();

    private final static String userName;

    private ThreadsManager() {

    }

    public static ThreadsManager getInstance(String user){
        userName = user;
        return instance;
    }
}

So I am trying to add the new String userName variable that is static (once for the class) and final (can not be changed at a second time)

My problem is that Eclips marks as an error the lines:

1) private final static String userName; saying to me that:

 The blank final field userName may not have been initialized

It seems that would that the field will be initialized (I can initialize it to null but since it is final...I can't initialize later in the constructor)

2) userName = user; say to me that:

 The final field ThreadsManager.userName cannot be assigned

So what is the best solution to handle this situation?

If I remove the final from the userName variable definition it seems to me that work well but then I can change this value but maybe I simply can not provide the setter method for this field so I prevent external changes....

Some ideas?

AndreaNobili
  • 40,955
  • 107
  • 324
  • 596
  • 1
    Without knowing anything, I think this class is a bad idea. Singleton and ThreadsManager? You shouldn't have Singletons, and threads are best left to the concurrent package. You probably don't know enough about threads to write concurrent code well. (Few can.) – duffymo Dec 19 '13 at 11:05
  • What is the point of the `userName`? Is it the name of the person using the Singleton? Please explain. –  Dec 19 '13 at 11:25
  • http://stackoverflow.com/questions/11126866/thread-safe-multitons-in-java – assylias Dec 19 '13 at 15:31

6 Answers6

3

I think you want a singelton 'with arguments'. This should explain it :

Singleton with Arguments in Java

Community
  • 1
  • 1
Oliver Watkins
  • 12,575
  • 33
  • 119
  • 225
1

Since this class is a Singleton then the name shouldn't really change too much. I would suggest just keeping it as a constant inside the class. If the name might change when the program is executed on different occasions then see Solution 2 below.

Solution 1:

public class ThreadsManager 
{
    private final static ThreadsManager instance = new ThreadsManager();
    private String userName;

    private ThreadsManager()
    {
       final String name = "Name";
       userName = name;
    }

    public static synchronized ThreadsManager getInstance(String user)
    {
       return instance;
    }
}

Solution 2:

If you really want to set the name of the Singleton and every time the program is execute the name might be different. Just add this method:

private String userName = null;

// Can only be set after Singleton is created and when userName is null.
public void setName(String n)
{
   if(userName == null)
      userName = n;
}

Don't make your getInstance() method have a parameter, that is a bad design. Every time someone, or you, tries to get an instance from your class they/you have to provide a parameter which will be 99% of the time be irrelevant.

1

It is not going to be singleton if you want multiple state of an instance of that class,

you could create a cache of Object keyed with user so it would still be singleton for same state asked

private final Map<String, ThreadsManager> instanceCache = Collections.synchronizedMap<String, ThreadsManager>();

Also make sure you don't leak memory if you have tons of states for this class

jmj
  • 237,923
  • 42
  • 401
  • 438
0

You can do it using a nested class.

public class ThreadsManager {

    private static String userName;

    private ThreadsManager() {

    }

    public static ThreadsManager getInstance(String user){
        if (userName == null)
             userName = user;
        // the holder's instance is only initialised at this point, 
        // after userName is set.
        return Holder.instance;
    }

    static class Holder {
        private final static ThreadsManager instance = new ThreadsManager();
    }
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • I didn't down vote but how do you handle two calls of getInstance with first passing "a" and second passing "b" it would cause confusion – jmj Dec 19 '13 at 11:13
  • @JigarJoshi one will win, static initialisation is thread safe. Given that would be a very confusing situation, I believe that is as good as it gets. – Peter Lawrey Dec 19 '13 at 11:17
  • 1
    @downvoter you are free to suggest what is wrong, or even better say how you would solve it. It is easier to downvote than have something constructive to add. – Peter Lawrey Dec 19 '13 at 11:18
0

First of all: private final static String userName may only be initialized inside the private constructor or during definition.

Secondly You may end up with a null instance, so you might do something like this:

public class ThreadsManager {

private final static ThreadsManager instance = new ThreadsManager();

private String userName;

private ThreadsManager() {

}

private ThreadsManager(String user) {
    userName = user;
}


public static ThreadsManager getInstance(String user){
    if(instance == null) {
        instance = new ThreadsManager(user);
    } else {
        Logger.getInstance().logWarning("User has already been set. Will continue with user ["+username+"].);
    }

    return instance;
    }
}

The handling of how to deal with a second user name handed needs some thinking. Overall you should try to keep the getInstance() method parameter free since it leads to the above mentioned problems.

Christian Kullmann
  • 558
  • 1
  • 7
  • 21
-1

How about

public class ThreadsManager {
    private final static ThreadsManager instance = new ThreadsManager();
    private static String userName;

public static synchronized ThreadsManager getInstance( String user ) {
    if ( username == null ) { userName = user; }
    return instance;
}

That would ensure userName is only set the first time.

It is, however, potentially very confusing semantics for a singleton to take a parameter that is ignored on subsequent getInstance()'s - possibly even race-condition-prone, depending on your use-case.

Cheers,

Anders R. Bystrup
  • 15,729
  • 10
  • 59
  • 55