4
\$\begingroup\$

Unfortunately, I can't use the Tomcat session for storing the key/value pairs for each user (restricted because it's an IVR domain-based project).

But I need the same functionality like a Tomcat session.

For every user I can get a unique id (call Id). So it's like the Tomcat sessionId.

I have developed the following class to achieve the Tomcat session functionality with my little knowledge of Java.

/** enum is used for Singleton instance  **/
public enum SessionObject {

    /** single instance  **/
    INSTANCE;

    /** ConcurrentHashMap to avoid external synchronization  **/
    /** Key as callId , value as HashMap to store the key value pairs  **/
    private ConcurrentHashMap<String , ConcurrentHashMap<String,String>> session =  new  ConcurrentHashMap<String , ConcurrentHashMap<String,String>>();

    /** Used to create a new Map when new call ivr is established in the first vxml file**/
    public void createSession(String callId){

        ConcurrentHashMap<String,String> callMap = new ConcurrentHashMap<String,String>();
        session.putIfAbsent(callId, callMap);
    }

    /** Used to get the existing stored values for the particular call Id **/
    public ConcurrentHashMap<String,String> getSession(String callId){

        return session.get(callId);
    }

    /** Used to remove the details of the particular call Id , when call is disconnected **/
    public void deleteSession(String callId){

        session.remove(callId);
    }

}

I think that the code above will be more fine tunable. Suggestions to improve the performance would be appreciated.

\$\endgroup\$
  • \$\begingroup\$ Concerning performance, there's no reason to believe that the Tomcat session is any faster. \$\endgroup\$ – maaartinus Sep 11 '14 at 20:12
7
\$\begingroup\$

This is pretty simple code, with not a lot of room for improvement.


You can make the session variable final, and it's good to do so:

private final ConcurrentHashMap<String, ConcurrentHashMap<String, String>> session = new ConcurrentHashMap<String, ConcurrentHashMap<String, String>>();

You can simplify createSession a bit by dropping the pointless local variable:

public void createSession(String callId) {
    session.putIfAbsent(callId, new ConcurrentHashMap<String,String>());
}

Some of the comments are really pointless, for example these:

/** enum is used for Singleton instance  **/
public enum SessionObject {

    /** single instance  **/
    INSTANCE;

You're using spaces in a non-standard way. Instead of this:

private ConcurrentHashMap<String , ConcurrentHashMap<String,String>> session =  new  ConcurrentHashMap<String , ConcurrentHashMap<String,String>>();

Don't put a space in front of commas, but put one after, like this:

private ConcurrentHashMap<String, ConcurrentHashMap<String, String>> session = new ConcurrentHashMap<String, ConcurrentHashMap<String, String>>();

It's good that you comment your methods. But instead of this:

/** Used to create a new Map when new call ivr is established in the first vxml file**/
public void createSession(String callId){

The correct format for JavaDoc comments is like this:

/**
 * Used to create a new Map when new call ivr is established in the first vxml file
 * 
 * @param callId the call id for which to create new session
 */
public void createSession(String callId) {

Discussion

Our discussion in comments is perhaps worth including in the answer itself.

Is it necessary to make the method argument as final ?

In your code, it's not necessary. But it's recommended. By setting method parameters final, you can prevent reassigning the value by accident. It's a bad practice to reassign the values of method parameters, for example in here: void fun(int something) { something = 3; }

Can you please tell me, why you put the session variable as final ?

It's a good practice to make member fields final, as much as possible. final variables cannot change, they are immutable. Being immutable makes a variable more reliable and robust: once created correctly, you cannot break it, for example by accidentally reassigning to a value that wasn't correctly validated.

I have used the enum for singleton . So my class is thread safe. Also I declared a session variable as private . So, Is it necessary to use the concurrent hashmap instead of ordinary hashmap in this case ?

The enum singleton guarantees that all threads will see the same INSTANCE. However, if you have multiple threads, they might access the session map concurrently, which would not work with a regular hashmap. So yes, session should be a ConcurrentHashMap, unless the rest of your implementation guarantees single-threaded access.

By adding the final, the session variable become immutable. But I am adding the key/value pairs in the 'cmap'. So the map will become mutable.

I meant in the more general sense. For example simple types like int would be truly immutable. With object types, only their reference, the variable pointing to them will be immutable. Although their content is still editable (why you mustn't forget), not being able to reassign them does give you some level of protection. So it's always good to make your members final whenever possible. When your solution requires some members to be not final, give it some thought to refactor your solution to make them final. This is not always possible, but you should at least try, always.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your answer. Is it necessary to make the method argument as final ? \$\endgroup\$ – Fisher Man Sep 12 '14 at 6:48
  • \$\begingroup\$ In your code, it's not necessary. But it's recommended. By setting method parameters final, you can prevent reassigning the value by accident. It's a bad practice to reassign the values of method parameters, for example in here: void fun(int something) { something = 3; } \$\endgroup\$ – janos Sep 12 '14 at 7:05
  • \$\begingroup\$ Thanks. Can you please tell me, why you put the session variable as final ? \$\endgroup\$ – Fisher Man Sep 12 '14 at 7:15
  • \$\begingroup\$ One more question . I have used the enum for singleton . So my class is thread safe. Also I declared a session variable as private . So, Is it necessary to use the concurrent hashmap instead of ordinary hashmap in this case ? \$\endgroup\$ – Fisher Man Sep 12 '14 at 7:19
  • 1
    \$\begingroup\$ Excellent answer . Thanks for posting the discussions in the answer.If possible , I will put n number of up votes. But I can't.So as usual only one. \$\endgroup\$ – Fisher Man Sep 12 '14 at 8:59
5
\$\begingroup\$

I agree with @janos's assessment of your code:

  • the singleton enum is done right
  • the sessions should be final
  • comments are 'off'

I would suggest you change the basic interface for your sessions though. Your session service should be a more 'opaque' one: your session 'client' has to first create a session, then get it. Also, you create a new ConcurrentHashMap instance each time you create a session, even if there is an instance available already.

I recommend removing the createSession method entirely, and just extending the getSession to be a lazy-create version:

public ConcurrentHashMap<String,String> getSession(final String callId){
    ConcurrentHashMap<String,String> candidateMap = session.get(callId);
    if (candidateMap == null) {
        //first time for this callId
        candidateMap = new ConcurrentHashMap<>();
        ConcurrentHashMap<String,String> race = session.putIfAbsent(callId, candidateMap );
        candidateMap = race != null ? race : candidateMap ;
    }
    return candidateMap ;
}

The above method solves a few problems:

  1. It eliminates the two-stage interaction with the manager: 1. create, 2. get
  2. It only creates a new ConcurrentHashMap when the session never existed before, so reduces the number of 'throw-away' maps
  3. It is still thread safe, and only if there is a (rare) race-condition is the map thrown out.

Also, note that the constructor for the ConcurrentHashSet uses the new-in-Java-7 'diamond operator' <> which saves a lot of retyping in the constructor call.

\$\endgroup\$
  • \$\begingroup\$ cmap stand for client map or concurrent map ? callId looks weird since l and I side-by-side looks so alike. \$\endgroup\$ – Marc-Andre Sep 11 '14 at 17:30
  • \$\begingroup\$ callId was inherited with the OP's code, and yes, cmap could be better, but ... well, OK, I'll change it. \$\endgroup\$ – rolfl Sep 11 '14 at 17:36
  • \$\begingroup\$ I should have added that those are so minor (callId sure is) that it's not really needed to do any change, IMO. Glad you still edited cmap, candidateMap looks way better ! (you forgot one ` after callId) \$\endgroup\$ – Marc-Andre Sep 11 '14 at 17:38
  • \$\begingroup\$ That's a lot of code to fix something that ain't broken (performance). Until we've identified this method as a bottleneck, this is premature optimization. Granted, it's more efficient than the original, but at the cost of readability. Consider that code is typically read many times during its lifetime. While the original is very easy to read for anyone, if I see this I would have to pause to take a closer look. Finally, and this might be a moot point in Java, but at least in theory, the second arg of .putIfAbsent(callId, _) shouldn't even be evaluated if the key exists. \$\endgroup\$ – janos Sep 11 '14 at 21:19
  • \$\begingroup\$ @janos - You are missing the part where I have completely eliminated a whole method, (createSession) and thus simplified all the 'client' code. This will simplify a lot of code, and put all the if/else logic in one place. The performance boost is only a bonus. Also, the _ argument to putIfAbsent(..) is always evaluated before putIfAbsent is called. \$\endgroup\$ – rolfl Sep 11 '14 at 21:55

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.