4
\$\begingroup\$

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));
                }
            });
}
\$\endgroup\$
1
  • \$\begingroup\$ It's the singleton which indicates problems with the code. \$\endgroup\$
    – Tom Hawtin - tackline
    Jan 28, 2013 at 17:20

5 Answers 5

3
\$\begingroup\$

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();
\$\endgroup\$
5
  • \$\begingroup\$ 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? \$\endgroup\$ Jan 28, 2013 at 18:28
  • 1
    \$\begingroup\$ @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. \$\endgroup\$
    – Edwin Buck
    Jan 28, 2013 at 19:28
  • \$\begingroup\$ As a proponent of functional programming, I disagree with the essence of your argument. Polymorphism and storing data in objects (mutable state!) aren't necessarily good things; in fact I would argue that they are more often than not bad. Source 1) en.wikipedia.org/wiki/Composition_over_inheritance 2) c2.com/cgi/wiki?KillMutableState \$\endgroup\$ Jun 18, 2015 at 17:14
  • \$\begingroup\$ @Sridhar-Sarnobat If you don't want objects (code and data together) and you don't want polymorphisim, then as a proponent of functional programming why use Java? Scheme or CommonLisp work just fine, and aren't encumbered with an entire library that's designed (more or less) to have these features you don't want to use. From those of us who came into Java from the C / Iterative side of programming, these features make it possible to write a lot of code (tens of millions of lines) in a maintainable way, something not easy to do from the functional side (I know, I've lived in both worlds). \$\endgroup\$
    – Edwin Buck
    Jun 18, 2015 at 18:25
  • \$\begingroup\$ In practice, the programming paradigm is only one aspect to consider when choosing a language. I've worked at some large companies (who I guess I am not allowed to mention) that are big on functional programming but are tied to java. They're not ready to make that leap to Scala etc. \$\endgroup\$ Jun 18, 2015 at 19:43
3
\$\begingroup\$

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:

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 28, 2013 at 19:23
  • \$\begingroup\$ @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.) \$\endgroup\$
    – palacsint
    Jan 29, 2013 at 1:02
0
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ The AndroidAsynchronousHttpClient executes the post in another thread. \$\endgroup\$ Jan 28, 2013 at 17:14
  • 1
    \$\begingroup\$ oh ok dont know much about android development :P \$\endgroup\$
    – p000ison
    Jan 28, 2013 at 17:22
0
\$\begingroup\$

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

\$\endgroup\$
0
\$\begingroup\$

I disagree completely with all the others in this thread, probably because I find Functional programs so much easier to reason about than Imperative Object-Oriented code. Making a method static is the first step to making java code functional-style.

When you make methods static, you make dependencies between inputs and outputs explicit without having to read the body of the method. It's a similar argument to why dependency injection is helpful.

I agree that inability to mock can be inconvenient for testing existing code, but the need to mock in the first place is a sign that your code has rigid dependencies.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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