Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm developing an Android app that performs several requests to a server using the AndroidAsynchronousHttpClient. One of these requests (as an example) is responsible to send the username and password. I created a class Sign that has a static method (see below) to which I provide a callback to handle the server's response. Is this considered a bad practice?

I'm asking this because I'm having a hard time testing with Mockito.

public class Sign {

public static void signin(String username, String password,
        final SigninResponseHandler responseHandler) {

    RequestParams params = new RequestParams();
    params.put("username", username);
    params.put("password", password);

    MyAPIClient.getInstance().post(path, params,
            new JsonHttpResponseHandler()
            {
                @Override
                public void onSuccess(JSONObject responseObject)
                {
                    String responseAPIStatus = responseObject.getString("status");
                    if (status.isEquals("success")
                    {
                        responseHandler.callback(true, null);
                    }

                }

                @Override
                public void onFailure(Throwable e, JSONObject errorResponse)
                {
                    responseHandler.callback(false, new CustomError(e));
                }
            });
}
share|improve this question

migrated from stackoverflow.com Jan 28 '13 at 17:37

This question came from our site for professional and enthusiast programmers.

2  
I think this question would be a better fit for codereview. –  Vivin Paliath Jan 28 '13 at 17:05
    
Posting there too. Didn't know about that :) –  Raphael Oliveira Jan 28 '13 at 17:08
    
where is codereview? –  happytime harry Jan 28 '13 at 17:13
    
It's the singleton which indicates problems with the code. –  Tom Hawtin - tackline Jan 28 '13 at 17:20
    
Don't cross post, flag, and we'll migrate it over. –  casperOne Jan 28 '13 at 17:36

4 Answers 4

up vote 3 down vote accepted

Overdoing static is a code smell. Basically you have to balance things.

Too much static code typically means not enough code in the objects, or not much object oriented structure. Since objects provide boundaries between the in-object code and the rest of the system, they allow "leverage points" for future change (and for testing). You can, for example, subclass an object to provide a variety of implementations of a particular class method.

With static methods, no such polymorphisim is possible. This can lead to switch statements embedded within the static method (to function on key fields, acting somewhat like the state pattern). The primary reason this tends to become a smell is that switch statements put the burden of code maintenance outside of the object. In extreme cases, it begins to feel like the object is not encapsulating it's behavior, but rather the switch statement dictates external behavior depending on the data fields within the object. Such code becomes difficult to maintain over time.

I would avoid static methods as much as reasonably possible; however, in some cases it is a better choice to use them. For example, one would not typically benefit from subclassing the java core types. So if I needed to write a method that operated on String objects, I would probably prefer to put that as a static method on a StringUtils class. On the other hand, if it were a class that I had written, I would probably go out of my way to stay more object-oriented.

In your example...

MyAPIClient.getInstance().post(path, params,
            new JsonHttpResponseHandler()
            {
                @Override
                public void onSuccess(JSONObject responseObject)
                {
                    String responseAPIStatus = responseObject.getString("status");
                    if (status.isEquals("success")
                    {
                        responseHandler.callback(true, null);
                    }

                }

                @Override
                public void onFailure(Throwable e, JSONObject errorResponse)
                {
                    responseHandler.callback(false, new CustomError(e));
                }
            });

is going to be very hard to test. If only you had made an interface on the MyAPIClient, you could do something like.

public SignIn(String username, String password,
    final SigninResponseHandler responseHandler,
    final MyAPIClient) {
   ...
}

which would make SignIn easily be unit testable, like so

SignIn signIn = new SignIn("bob", "supersecret", new JsonHttpResponseHandler() {...},
    new MockAPI() {... });
signIn.perform();

while the mainline code would look like

SignIn signIn = new SignIn("bob", "supersecret", new JsonHttpResponseHandler() {...},
    MyAPIClient.getInstance());
signIn.perform();
share|improve this answer
    
Thank you so much Edwin for your answer but now I'm concerned about one Class file for each request type I have in my application. Two if you consider the response handles interface. Isn't that strange? For example, I have two requests: product/list and product/buy. They shouldn't be in the same file? –  Raphael Oliveira Jan 28 '13 at 18:28
1  
@RaphaelOliveira It is normal to have concerns, and eventually we all settle for the best choice we can afford out of a number of progressively more expensive solutions. Your second question would be better served as a second question. The commentary sections at the end of a question are not large enough to really address a second follow up question. –  Edwin Buck Jan 28 '13 at 19:28

Yes, it's a smell: Hard-to-Test Code

I'd start with the following:

  1. Create an interface for MyApiClient (usually it's easier to mock interfaces than classes),
  2. Remove the static modifier of signin,
  3. Create a constructor in Sign which get a MyApiClient instance and store it in a field,
  4. Modify the signin method to use the instance above.
public class Sign {

    private final MyApiClient client;

    public Sign(final MyApiClient client) {
        this.client = client;
    }

    public void signin(final String username, final String password, 
            final SigninResponseHandler responseHandler) {
        ...
        client.post(path, params, ...

Here you can create a Sign object with a mocked MyApiClient which is easier to test.

Some other notes:

  1. Sign does not seem a good class name. From Clean Code, page 25:

    Classes and objects should have noun or noun phrase names like Customer, WikiPage, Account, and AddressParser. [...] A class name should not be a verb.

  2. MyApiClient also should be renamed something meaningful.

Some useful readings:

share|improve this answer
    
Thanks for your answer palacsint. One doubt, why MyApiClient should be an interface? At the moment it is a singleton and extends AsyncHttpClient to do custom url behavior. The guys recommend to use static methods loopj.com/android-async-http. –  Raphael Oliveira Jan 28 '13 at 19:23
    
@RaphaelOliveira: I think they suggest static because "to make it easy to communicate with Twitter’s API". On the other hand it makes testing harder. (I've updated the answer too.) –  palacsint Jan 29 '13 at 1:02

I see nothing bad with this code. I mean as far as I can see you do not want to store any data, you just want to get the answer from the server, but maybe you want to handle this by an other thread because it can cause delays.

share|improve this answer
    
The AndroidAsynchronousHttpClient executes the post in another thread. –  Raphael Oliveira Jan 28 '13 at 17:14
1  
oh ok dont know much about android development :P –  p000ison Jan 28 '13 at 17:22

Each new call to this method creates a new instance of the interface thus all calls to this method use a resource (JsonHttpResponseHandler) different. But you deveira mark the method as "synchronized" for sharing the object "SigninResponseHandler"

Synchronized on oracle

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.