0
\$\begingroup\$

I understand and apply the DRY principle but I have a hard time getting this DRYer than it is right now.

/**
 * The Caller class is used to translate method calls to rest calls.
 *
 * It uses a low level AsyncHttpClient to execute the actual calls.
 *
 * It uses callbacks which are executed when the response is received.
 */

public class Caller {

    // the (rest) base url to where the calls must be directed
    private static final String baseUrl = "http://very.secret.domain.org:1337/";

    // singleton
    private static final Caller instance = new Caller();

    // the client which does the actual (low level) calls
    private static AsyncHttpClient client = new SyncHttpClient();

    // the (rest) method part of the url 
    private static final String registerUrl = baseUrl + "createUser/";
    private static final String verifyUrl = baseUrl + "searchUser/";

    // singleton
    private Caller() {
    }

    synchronized static Caller getInstance() {
        return instance;
    }

    // creates a user with given parameters 
    synchronized void getUser(final ShareActivity activity, // the activity which calls this method 
                                                            // and expects the result (asynchronously)  
                              String idToken, String name, String email) {

        //arguments
        Map map = new HashMap();

        map.put("token", idToken);
        map.put("name", name);
        map.put("email", email);

        //casted
        RequestParams params = new RequestParams(map);

        // call to get
        get(registerUrl, 
                params, 
                new JsonHttpResponseHandler() {//the handler

                    // when response recieved
            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONObject user) {

                // pass the (json) User object to the (calling and passed) activity 
                activity.handleUser(user);
            }

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONArray array) {

                // should not happen (the rest endpoint doesn't produce arrays)
                throw new RuntimeException("array in stead of object");
            }
        });
    }

    // more or less the same as above (except for added comments)
    synchronized void getUser(final ShareActivity activity, final String idToken) {

        // the same map again with the same first (and here only) parameter
        // I really think this can be done DRYer
        Map map = new HashMap();

        map.put("token", idToken);

        RequestParams params = new RequestParams(map);

        // same call
        get(verifyUrl, params, 
                new JsonHttpResponseHandler() { // other handler

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONObject user) {

                // this call (with different url, 
                // more or less the same map 
                // does other work in the same activity)
                // that's to much of same in the same sentence I suspect ;-)
                if(user!=null){
                    activity.handleUser(user);
                }else {
                    activity.registerUser();
                }
            }

            @Override
            public void onSuccess(int statusCode, Header[] headers, JSONArray array) {

                // shouldn't happen either
                throw new RuntimeException("array in stead of object");
            }
        });
    }

    // the lowlevel call
    private static void get(String url, RequestParams params, AsyncHttpResponseHandler responseHandler) {
        client.get(url, params, responseHandler);
    }
}

As can be seen, the getUser() is overloaded. There are two different use cases which differ in URL, the content of map and the handler.

The get method on the bottom is reused but I was wondering whether it is possible to get this code cleaner (or DRYer in any case...).

\$\endgroup\$
  • 2
    \$\begingroup\$ Your question could really benefit from a description of what it is the code actually does,. \$\endgroup\$ – forsvarir Mar 11 '17 at 11:33
2
\$\begingroup\$

General Hints

Naming

Your code does not respect Java Naming Conventions. The constants (static final members) names should be ALL_UPPER_CASE.

Singelton Pattern

Do not use the Singelton pattern. It makes it hard to reuse the code using this one in general and to test (in UnitTests) in particular.

BTW: you did it wrong.

Comments

Comments should explain why the code is like it is.

Your comments simply lie.

Following your comments the first method in JsonHttpResponseHandler should throw the exception, not the second.

This problem is quite common in code under development. Someone changes the behavior of the code but does not adjust the comments, turning them into lies.

So just delete the comments.

Supporting DRY

Often you don't have pure duplicated code. In most cases you have similar looking code which you usually quite easily can change to equal code by simple refactorings and/or by applying design patterns.

This goes basically in 3 steps.

  1. identify similar code
  2. make similar code the same
  3. remove the duplication

Since this process is expected not to change the behavior of your application you should better have UnitTest that fix the desired behavior so that you get immediate response when you broke something.

Identify similar code

The biggest block of duplication is the anonymous inner class of type JsonHttpResponseHandler which is created twice with the almost same content.

The only difference is the check for the user being null.

Make similar code the same

In this step you convert individual similar parts of your code to look exactly the same. Avoid typing as much as possible and rely on your IDEs refactorings like extract to local variable, rename in file and alike.

If the execution context of the first JsonHttpResponseHandler implementation guarantees that user never references null then you could simply copy the null check to it to make the same as the second.

Skip the rest of the section if that applies.


If not you should refactor out the difference. You could do so by introducing a new interface:

public class Caller {
  private interface ActivityCaller{
     void handle(ShareActivity activity,JSONObject user);
  }

Then you implement the interface with the respective behavior before creating the instance of JsonHttpResponseHandler:

// first version
RequestParams params = new RequestParams(map);
ActivityCaller activityCaller = (a,user)-> a.handleUser(user); // java8 Lambda Style, lambda parameter name 'a' must differ from the methods parameter name
get(registerUrl, params, new JsonHttpResponseHandler() {
  @Override
  public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
     activityCaller.handleUser(activity,user);
  }
  // .. no changes after here

// second versionRequestParams params = new RequestParams(map);
ActivityCaller activityCaller = new ActivityCaller(){ // traditional annonymous inner class
 @Override
  public void handle(ShareActivity activity,JSONObject user){
    if(user!=null){
        activity.handleUser(user);
    }else {
        activity.registerUser();
    }
  }
}
get(registerUrl, params, new JsonHttpResponseHandler() {
  @Override
  public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
     activityCaller.handleUser(activity,user);
  }
  // .. no changes after here

Remove the duplication

Now both anonymous inner class implementations of JsonHttpResponseHandler are exactly the same. Use your IDEs convert anonymous to inner class refactoring to replace both occurrences.

This creates a new inner class:

// assumed you copied the `null` check
public class Caller {
  private static class JsonHttpResponseHandlerImpl{
    private final ShareActivity activity; // if you copied the null check
    JsonHttpResponseHandlerImpl(ShareActivity activity){
      this.activity = activity;
    }
    @Override
    public void onSuccess(int statusCode, Header[] headers, JSONObject user) {
        if(user!=null){
            activity.handleUser(user);
        }else {
            activity.registerUser();
        }
    }
    @Override
    public void onSuccess(int statusCode, Header[] headers, JSONArray array) {
        throw new RuntimeException("array in stead of object");
    }
  }

and both of your methods change to:

   RequestParams params = new RequestParams(map);
   get(registerUrl, params, new JsonHttpResponseHandlerImpl(activity));

As a result you have even more common code in both methods which you could easily move to the common get method.

What's left in your getUser() methods it the creation of the map with different content and the implementations of the new ActivityCaller interface if you decided for this way.

\$\endgroup\$

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.