WeatherForecastRequest and its builder

WeatherForecastRequest is only simple HTTP parameters wrapper. It is using to get forecast from the openweathermap.

At first here is utility class (used to simplify actions to post http request, receive response from server and convert it to String):

// import packages from org.apache.http.***

public class HttpRequestor {

    public static interface ArgumentsProvider {
        List getArguments();
    }

    private static final HttpClient httpClient = new DefaultHttpClient();

    private final List arguments;
    private final String url;

    public HttpRequestor(String url, ArgumentsProvider argumentsProvider) {
        this.url = url;
        this.arguments = argumentsProvider.getArguments();
    }

    public HttpRequestor(String url, List arguments) {
        this.url = url;
        this.arguments = arguments;
    }

    public HttpRequestor(String url) {
        this.url = url;
        this.arguments = new ArrayList();
    }

    public String request() throws IOException {
        HttpPost httpPost = new HttpPost(url);
        httpPost.setEntity(new UrlEncodedFormEntity(arguments));
        HttpResponse httpResponse = httpClient.execute(httpPost);
        return EntityUtils.toString(httpResponse.getEntity());
    }

}

And WeatherForecastRequest itself:

```
import com.google.common.base.Objects;
import com.google.common.base.Optional;
// other imports skipped

public class WeatherForecastRequest implements HttpRequestor.ArgumentsProvider, Parcelable {

public static Builder builder() {
return new Builder();
}

public static class Builder {
private Optional query;
private Optional units;
private Optional mode;
private Optional numberOfDays;

private Builder() {
query = Optional.absent();
units = Optional.absent();
mode = Optional.absent();
numberOfDays = Optional.absent();
}

public Builder query(String query) {

And especially I want your opinion about proper order of fields and methods (static and not-static) in the WeatherForecastRequest class.

That's the easiest part, but I think it should be the least of your concerns.
See it explained in this checkstyle rule (based on JLS, the Java Language Specification):


According to Code Conventions for the Java Programming Language , the
parts of a class or interface declaration should appear in the
following order:



-
Class (static) variables. First the public class variables, then the
protected, then package level (no access modifier), and then the
private.

-
Instance variables. First the public class variables, then
the protected, then package level (no access modifier), and then the
private.

-
Constructors

-
Methods


Make HttpRequestor more solid

The current implementation has a leaky interface.
What I mean by that,
there are too many ways to construct this class:

• construct with url and ArgumentsProvider

• construct with url and List

• construct with url

This might seem "nice" to do so,
but it adds a lot of complications:

-
You must ensure that whichever constructor is used, the created object will be complete and usable. The more constructors you provide, the more work for you.

-
Users of the class, seeing these options, might ask themselves: "which constructor should I use?" The more constructors you provide, the more work for the users.

I recommend to choose between the 1st and 2nd constructor, but don't expose both.

If you really need the 3rd (url only) constructor, then make it call the other constructor.

Btw, if you need an empty list, don't do it like this:

this.arguments = new ArrayList();

Do it like this instead:

this.arguments = Collections.emptyList();

With the declarations reordered to follow JLS, the class would become something like this:

class HttpRequestor {

    private static final HttpClient httpClient = new DefaultHttpClient();

    private final String url;
    private final List arguments;

    public static interface ArgumentsProvider {
        List getArguments();
    }

    public HttpRequestor(String url, ArgumentsProvider argumentsProvider) {
        this.url = url;
        this.arguments = argumentsProvider.getArguments();
    }

    public String request() throws IOException {
        HttpPost httpPost = new HttpPost(url);
        httpPost.setEntity(new UrlEncodedFormEntity(arguments));
        HttpResponse httpResponse = httpClient.execute(httpPost);
        return EntityUtils.toString(httpResponse.getEntity());
    }
}

Improving WeatherForecastRequest

First of all, the name is misleading:
the main function of this class is that it's an ArgumentsProvider,
so I would recommend to rename to WeatherForecastRequestArgumentsProvider.

Instead of using String constants to build the query arguments,
I recommend an enum like this:

enum QueryParam {
    QUERY("q"),
    MODE("mode"),
    UNITS("units"),
    COUNT("cnt");

    private final String name;

    private QueryParam(String name) {
        this.name = name;
    }
}

Here's a sample implementation, without the Parcelable part,
using an enum, and a cleaner implementation of the Builder pattern:

```
class WeatherForecastRequestArgumentsProvider implements HttpRequestor.ArgumentsProvider {

enum QueryParam {
QUERY("q"),
MODE("mode"),
UNITS("units"),
COUNT("cnt");

private final String name;

private QueryParam(String name) {
this.name = name;
}
}

private final List arguments;

private WeatherForecastRequestArgumentsProvider(Builder builder) {
arguments = new ArrayList(builder.getArguments());
}

@Override
public List getArguments() {
return arguments;
}

public static Builder builder() {
return new Builder();
}

public static class Builder {

Map arguments = new HashMap<>();

public Builder query(String query) {
arguments.put(QueryParam.QUERY, query);
return this;
}

public Builder units(String units) {
arguments.put(QueryParam.UNITS, units);
return this;
}

public Builder mode(String mode) {
arguments.put(QueryParam.MODE, mode);
return this;
}

public Builder numberOfDays(String numberOfDays) {
arguments.put(QueryParam.COUNT, numberOfDays);
return this;
}

public List getArguments() {
List nonNullArguments = new ArrayList();
for (Map.Entry entry : arguments.entrySet()) {
String value = entry.getValue();
if (value != null) {
nonNullArguments.add(new BasicNameValuePair(entry.getKey().name, value));
}
}
return Collections.unmodifiableList(nonNullArguments);

StackExchange Code Review Q#64778, answer score: 2