trjhtr

I have a piece of code that will have to fetch the token to call a different API. The token has an expiry of 10 hours; after 10 hours, it has to re-fetch the token.

I have created a singleton class to do it. Is there anything wrong with this code?

 import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;

 public class CredentialsHolder 

 private static CredentialsHolder instance = null;
 private String loginToken;
 private Date loginTokenExpiry;

 private static final String API_BASE_URL_V1 = "https://api.com/rest";
 private static final String API_LOGIN_TOKEN_PATH = "token";
 private static final String API_USER_NAME = "userName";
 private static final String API_PASSWORD = "password";

 private final Lock lock = new ReentrantLock();

 public static synchronized CredentialsHolder getInstance() 
 if (instance == null)
 instance = new CredentialsHolder();
 return instance;
 

 private CredentialsHolder() 
 setAPICredentials();
 

 private void refreshToken() 
 setAPICredentials();
 

 private void setAPICredentials() 
 if (Calendar.getInstance().after(toCalendar(loginTokenExpiry))) 
 Map headers = new HashMap();
 headers.put("userName", API_USER_NAME);
 headers.put("password", API_PASSWORD);
 Map criteria = new HashMap();
 criteria.put("headers", headers);
 criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);
 Map response = new ExternalAPICaller().httpGet(criteria);
 Map responseHeaders = ((Map) response.get("headers"));
 System.out.println(responseHeaders);
 loginToken = (String) (responseHeaders.get("Token"));
 try 
 loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse((String) (responseHeaders.get("TokenExpiry")));
 catch (ParseException e) 
 e.printStackTrace();
 loginTokenExpiry = null;
 
 
 

 public String getAccessToken() 
 if (Calendar.getInstance().after(toCalendar(loginTokenExpiry))) 
 if (lock.tryLock()) 
 try 
 refreshToken();
 finally 
 lock.unlock();
 
 
 
 return loginToken;
 

 public static Calendar toCalendar(Date date)
 Calendar cal = Calendar.getInstance();
 cal.setTime(date);
 cal.add(Calendar.HOUR, -1);
 return cal;
 
 

I have used ReentrantLock to not allow the refresh happen simultaneously by multiple threads. So if a refresh is happening once, the other will continue. (I am trying to refresh the token 1 hour ahead before expiry).

Singleton

There is a definite argument that a Singleton is an anti-pattern. I won't go into it too much, but just to make it not link-only:

1. Singletons are hard to unit test, as of course you can only have one.


2. It's hard to reuse a Singleton if you suddenly find yourself needing two. E.g. if you have a similar API, you have to copy the whole Singleton even though most of the code is the same.

What should you do instead? Alternatives to Singleton.

In particular, consider a pattern like

CredentialsHolder apiCredentials = CredentialsHolder.getInstance("api_credentials.txt");

Behind the scenes, this might do a lookup in an instance table. If found, use it. If not found, create it.

So

 if (instance == null)
 instance = new CredentialsHolder();

becomes

 CredentialsHolder instance = credentials.get(apiFileName);
 if (instance == null) 
 instance = connectToApi(apiFileName);
 credentials.put(apiFileName, instance);
 

Configure outside the code

This also moves the login information out of your code and into configuration. This is often helpful in that you want to share your code but not your configuration. That is to say, your login information is private, but you are posting your code publicly. What if you forgot to obscure the info?

Curly brackets

I personally prefer the block form of control structures even when there is only a single statement. If you do use the single statement form, consider putting it on the same line, e.g.

 if (instance == null) instance = new CredentialsHolder();

But of course, that won't work in my revised code, which has a two statement block.

The counter-argument is that it's not actually necessary in your original code. But neither is it harmful in your original code. I find a simple bright line rule of always using them better than trying to evaluate each situation. This is especially true as the coding situation often changes quickly and in surprising ways. Using the will always work.

Paragraphing

 Map headers = new HashMap();
 headers.put("userName", API_USER_NAME);
 headers.put("password", API_PASSWORD);
 Map criteria = new HashMap();
 criteria.put("headers", headers);
 criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);
 Map response = new ExternalAPICaller().httpGet(criteria);
 Map responseHeaders = ((Map) response.get("headers"));
 System.out.println(responseHeaders);
 loginToken = (String) (responseHeaders.get("Token"));
 try 
 loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse((String) (responseHeaders.get("TokenExpiry")));
 catch (ParseException e) 
 e.printStackTrace();
 loginTokenExpiry = null;
 

That's a wall of code, which makes it harder to read. Consider

 Map<String, String> headers = new HashMap<>();
 headers.put("userName", API_USER_NAME);
 headers.put("password", API_PASSWORD);

 Map<String, Map<String, String>> criteria = new HashMap<>();
 criteria.put("headers", headers);
 criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);

 Map<String, Map<String, String> response = new ExternalAPICaller().httpGet(criteria);
 Map<String, String> responseHeaders = response.get("headers");
 System.out.println(responseHeaders);

 loginToken = responseHeaders.get("Token");
 try 
 loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse(responseHeaders.get("TokenExpiry"));
 catch (ParseException e) 
 e.printStackTrace();
 loginTokenExpiry = null;
 

Now it's much easier to see what code goes with what, because the blank lines (vertical whitespace) separate the blocks of code.

You also might consider moving the try/catch block into a separate method. Then you could say something like

 loginTokenExpiry = parseTokenExpiry(responseHeaders.get("TokenExpiry"));

Don't use raw types

I changed all the raw types to parameterized types. It's possible that that only works for the ones that you create, like headers and criteria, but I changed all of them. Change the latter ones back if ExternalAPICaller won't work with them. And then tell these External people to fix their API.

This eliminates the need to cast the results of get and avoids a compiler warning in some configurations.