trjhtr

This method initiates a new connection with the thrift client and returns a profile object

 private TResume fetchProfileObj(int userId) 
 try
 TTransport transport = new TSocket("172.XX.X.XXX”, 9311); 
 transport.open();
 TProtocol protocol = new TBinaryProtocol(transport);
 Map<String, String> ids = new HashMap<String, String>();
 ids.put("requestId", "settingsService");
 TResumeService.Client resumeServiceClient = new TResumeService.Client(protocol);
 TResume profileObj = resumeServiceClient.getFullActiveProfileFromUserId(ids, 105, userId, "resman5_1");
 transport.close();
 return profileObj;
 catch(Exception e) 
 e.printStackTrace();
 return null;
 

This method updates the profile object recieved from the fetchProfile method

private void updateProfileFlags(TResume profile,int recruiterJobAlert) 
 try
 TTransport transport = new TSocket("172.XX.X.XXX", 9321); 
 transport.open();
 TProtocol protocol = new TBinaryProtocol(transport);
 Map<String, String> ids = new HashMap<String, String>();
 ids.put("requestId", "settingsService");
 TUpdateResume.Client updateResumeClient = new TUpdateResume.Client(protocol);
 profile.getUser().setResdexVisibility(recruiterJobAlert>0?"a":"c");
 updateResumeClient.saveProfile(ids, 105, profile.getProfile(), "now", "resman5_1", null);
 transport.close();
 catch(Exception e) 
 e.printStackTrace();
 

Now calling the above methods

public void myTestMethod()
updateProfileFlags(fetchProfileObj(userId),settings.getRecruiterJobAlert());

1) Extract the duplicated map into a constant field (that'll remove 2 lines from both method) :

private static final Map<String, String> IDS = Collections.singletonMap("requestId", "settingsService");

2) Extract the creation of the TResumeService.Client and TUpdateResume.Client in separate methods looking like this :

private TResumeService.Client createService(TTransport transport) 
 return new TResumeService.Client(new TBinaryProtocol(transport));

3) Do not close your TTransport in the try block, indeed if an exception is thrown it'll never be closed, use the finally block for this.

4) Do not catch Exception to simply use printStackTrace, it's bad practice as the calling method won't know if (nor how) the method failed. If the methods cannot manage the exception... they should rethrow it. If having a general Exception object being thrown around does not suit you, you may wrap it inside an IOException for example like this :

try
 TTransport transport = new TSocket("172.XX.X.XXX”, 9311); 
 transport.open();
 // ....
 return profileObj;
 catch(Exception e) 
 throw new IOException(e);

5) Use a whitespace after commas and add some space in the following expression (recruiterJobAlert>0?"a":"c" to make it more readable

6) Put the string and 105 and 9311 "magic" values in properly named constants.

7) It's useless to store something in a variable and then simply return it, you should consider removing such assigment.

At this point, you'll have the following code :

private static final String RESMAN = "resman5_1";

private static final Map<String, String> IDS = Collections.singletonMap("requestId", "settingsService");

private TResumeService.Client createResumeService(TTransport transport) 
 return new TResumeService.Client(new TBinaryProtocol(transport));


private TUpdateResume.Client createUpdateService(TTransport transport) 
 return new TUpdateResume.Client(new TBinaryProtocol(transport));


private TResume fetchProfileObj(int userId) throws IOException 
 try
 TTransport transport = new TSocket("172.XX.X.XXX”, PORT); 
 transport.open();
 TResumeService.Client resumeServiceClient = createResumeService(transport);
 return resumeServiceClient.getFullActiveProfileFromUserId(IDS, SOME_CONSTANT, userId, RESMAN);
 catch(Exception e) 
 throw new IOException(e);
 finally 
 transport.close();
 


private void updateProfileFlags(TResume profile, int recruiterJobAlert) throws IOException 
 try
 TTransport transport = new TSocket("172.XX.X.XXX", PORT); 
 transport.open();
 TUpdateResume.Client updateResumeClient = createUpdateService(transport);
 profile.getUser().setResdexVisibility(recruiterJobAlert > 0 ? "a" : "c");
 updateResumeClient.saveProfile(IDS, SOME_CONSTANT, profile.getProfile(), "now", RESMAN, null);
 catch(Exception e) 
 throw new IOException(e);
 finally 
 transport.close();
 

Maybe you'll find that's enough of an improvement but there is still 6-7 lines duplicated between the 2 methods.

Let's dig a bit more and go functionnal :

We can see that the 2 methods look like this :

try
 TTransport transport = new TSocket("172.XX.X.XXX”, PORT); 
 transport.open();
 // something that varies here
 catch(Exception e) 
 throw new IOException(e);
 finally 
 transport.close();

The something that varies could be abstracted with a function...

Since Java 8 we can pass objects that are very tiny wrappers around functions, they are called lambdas.

Let's take a look at the package storing all those objects : https://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html
We sadly cannot use Function as it cannot throw exception... there is a Callable interface in java.util.concurrent but we cannot use it as it doesn't take any parameter... so let's use a new functional interface that suits our needs :

public interface ApplicationOverTransport<R> 
 R call(TTransport transport) throws Exception;

This interface would simply replace the variable part, we can now create a simple method that will be usable everytime you need a TTransport that is opened, do some actions and closed :

T applyOverTransport(ApplicationOverTransport<T> method) throws IOException 
 try
 TTransport transport = new TSocket("172.XX.X.XXX”, PORT); 
 transport.open();
 return method.call(transport);
 catch(Exception e) 
 throw new IOException(e);
 finally 
 transport.close();
 

By using lambda expressions, the methods could then look like this :

private TResume fetchProfileObj(int userId) throws IOException 
 return applyOverTransport(transport -> 
 TResumeService.Client resumeServiceClient = createService(transport);
 return resumeServiceClient.getFullActiveProfileFromUserId(IDS, SOME_CONSTANT, userId, RESMAN);
 );


private void updateProfileFlags(TResume profile, int recruiterJobAlert) throws IOException 
 applyOverTransport(transport -> 
 TUpdateResume.Client updateResumeClient = createService(transport);
 profile.getUser().setResdexVisibility(recruiterJobAlert > 0 ? "a" : "c");
 updateResumeClient.saveProfile(IDS, SOME_CONSTANT, profile.getProfile(), "now", RESMAN, null);
 return null;
 );

I am not sure how many lines of code can be reduced but one thing is for sure: whenever you encounter a resource that can be closed, you should use Java try-with-resources feature (since java 7). Not only this feature saves you the close() operation (one line less!) but also ensures that the resource is properly closed by the end of the try block no matter if it ended successfully or not (the compiler adds a finally clause). in the current code, if an exception is thrown, the Ttransport resource is not closed properly.

I looked at the thrift javadoc and indeed from version 0.10.0, Ttransport is auto-closeable. So the code on both methods should be

try (TTransport transport = new TSocket("172.XX.X.XXX", 9321)) 
 transport.open();
 ... // do not call close()!!
 // do not call close()!! it is added by the compiler
 catch(Exception e) 
 e.printStackTrace();

Regarding reducing duplicated lines, I can offer that you make a method to create the ids map. so if the contents of the map need to be modified due to modifications in the requirements or API, there will be only one place to modify them. creating constant variables instead of embedded literals is also considered best practice. This can apply to other literals used in the code

public static final String PROFILE_REQUEST_KEY = "requestId";
public static final String PROFILE_REQUEST_VALUE = "settingsService";
public static final int PROFILE_WHATEVER_105_MEANS = 105;
public static final String PROFILE_WHATEVER_RESMAN5_1_MEANS = "resman5_1";

public Map<String, String> getProfileRequestProperties() 
 return Collections.singletonMap(PROFILE_REQUEST_KEY, PROFILE_REQUEST_VALUE);

now, since we eliminated close(), we can return the value direct from the call to the client, eliminating the creation of the profileObj reference:

return resumeServiceClient.getFullActiveProfileFromUserId(
 getProfileRequestProperties(),
 PROFILE_WHATEVER_105_MEANS,
 userId,
 PROFILE_WHATEVER_RESMAN5_1_MEANS);

so, I ma not sure how many LOC are now present, but im my eyes, the code is clearer and more maintainable and extensible.