3
\$\begingroup\$

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<? extends NameValuePair> getArguments();
    }

    private static final HttpClient httpClient = new DefaultHttpClient();

    private final List<? extends NameValuePair> arguments;
    private final String url;

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

    public HttpRequestor(String url, List<? extends NameValuePair> arguments) {
        this.url = url;
        this.arguments = arguments;
    }

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

    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<String> query;
        private Optional<String> units;
        private Optional<String> mode;
        private Optional<String> numberOfDays;

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

        public Builder query(String query) {
            this.query = Optional.fromNullable(query);
            return this;
        }

        public Builder units(String units) {
            this.units = Optional.fromNullable(units);
            return this;
        }

        public Builder mode(String mode) {
            this.mode = Optional.fromNullable(mode);
            return this;
        }

        public Builder numberOfDays(String numberOfDays) {
            this.numberOfDays = Optional.fromNullable(numberOfDays);
            return this;
        }

        public WeatherForecastRequest build() {
            return new WeatherForecastRequest(this);
        }
    }


    public static final Creator<WeatherForecastRequest> CREATOR = new Creator<WeatherForecastRequest>() {
        @Override
        public WeatherForecastRequest createFromParcel(Parcel source) {
            return new WeatherForecastRequest(source);
        }

        @Override
        public WeatherForecastRequest[] newArray(int size) {
            return new WeatherForecastRequest[size];
        }
    };

    private final List<NameValuePair> arguments;

    private WeatherForecastRequest(Builder builder) {
        arguments = new ArrayList<NameValuePair>();
        addArgumentIfNotNull(arguments, ForecastParameters.QUERY, builder.query);
        addArgumentIfNotNull(arguments, ForecastParameters.UNITS, builder.units);
        addArgumentIfNotNull(arguments, ForecastParameters.MODE, builder.mode);
        addArgumentIfNotNull(arguments, ForecastParameters.COUNT, builder.numberOfDays);
    }

    private void addArgumentIfNotNull(List<NameValuePair> arguments, String argumentName, Optional<String> argumentValue) {
        if (argumentValue.isPresent()) {
            arguments.add(new BasicNameValuePair(argumentName, argumentValue.get()));
        }
    }

    @Override
    public List<? extends NameValuePair> getArguments() {
        return new ArrayList<NameValuePair>(arguments);
    }

    public WeatherForecastRequest(Parcel source) {
        int numberOfArguments = source.readInt();
        arguments = new ArrayList<NameValuePair>(numberOfArguments);
        for (int i = 0; i < numberOfArguments; ++i) {
            String name = source.readString();
            String value = source.readString();
            NameValuePair argument = new BasicNameValuePair(name, value);
            arguments.add(argument);
        }
    }

    @Override
    public void writeToParcel(Parcel out, int flags) {
        int numberOfArguments = arguments.size();
        out.writeInt(numberOfArguments);
        for (int i = 0; i < numberOfArguments; ++i) {
            NameValuePair argument = arguments.get(i);
            out.writeString(argument.getName());
            out.writeString(argument.getValue());
        }
    }

    @Override
    public int describeContents() {
        return 0;
    }

    @Override
    public String toString() {
        return Objects.toStringHelper(this)
                .add("arguments", arguments)
                .toString();
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == this) {
            return true;
        }
        if ((obj == null) || !(obj instanceof WeatherForecastRequest)) {
            return false;
        }
        WeatherForecastRequest other = (WeatherForecastRequest) obj;
        return Objects.equal(arguments, other.arguments);
    }

    @Override
    public int hashCode() {
        return Objects.hashCode(arguments);
    }
}


public class ForecastParameters {
    public static final String QUERY = "q";
    public static final String MODE = "mode";
    public static final String UNITS = "units";
    public static final String COUNT = "cnt";
}

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

\$\endgroup\$
0

3 Answers 3

2
\$\begingroup\$

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:

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

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

  3. Constructors

  4. 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<? extends NameValuePair>
  • 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<NameValuePair>();

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<? extends NameValuePair> arguments;

    public static interface ArgumentsProvider {
        List<? extends NameValuePair> 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<NameValuePair> arguments;

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

    @Override
    public List<? extends NameValuePair> getArguments() {
        return arguments;
    }

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

    public static class Builder {

        Map<QueryParam, String> 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<NameValuePair> getArguments() {
            List<NameValuePair> nonNullArguments = new ArrayList<NameValuePair>();
            for (Map.Entry<QueryParam, String> entry : arguments.entrySet()) {
                String value = entry.getValue();
                if (value != null) {
                    nonNullArguments.add(new BasicNameValuePair(entry.getKey().name, value));
                }
            }
            return Collections.unmodifiableList(nonNullArguments);
        }

        public WeatherForecastRequestArgumentsProvider build() {
            return new WeatherForecastRequestArgumentsProvider(this);
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ QUERY, MODE, UNITS, COUNT are only constants. What benefit you get by making them enumeration? \$\endgroup\$ Commented Oct 5, 2014 at 17:52
  • 1
    \$\begingroup\$ Type safety. With an enum, you cannot use just any String constant by mistake, only what's declared in the enum. It's also more efficient when used in a Map like I did, because of a faster hashCode implementation. It's not a huge concern in your code, but it's a good practice in general. \$\endgroup\$
    – janos
    Commented Oct 5, 2014 at 17:57
1
\$\begingroup\$
    public static interface ArgumentsProvider {
        List<? extends NameValuePair> getArguments();
    }

I'd use Guava's Supplier for better interoperability. Of course, I'm assuming that everyone uses Guava.

    public HttpRequestor(String url, List<? extends NameValuePair> arguments) {
        this.url = url;
        this.arguments = arguments;
    }

A public method should copy the incomming list so it can't get mutated later from the outside. Unless you're using ImmutableList.

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

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

This is a case where Optional just causes more work without any benefit. Actually, all uses in Java are like that. It's just that sometimes it can save you from an unexpected NPE. Sometimes. Maybe.

public class ForecastParameters {
    public static final String QUERY = "q";
    public static final String MODE = "mode";
    public static final String UNITS = "units";
    public static final String COUNT = "cnt";
}

I'd consider using an enum here. You could use an EnumMap instead of the arguments list.

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

No idea... what about sticking with Google Java Style? Basically, it says that you could do whatever you want as long as you do it consistently. :D

Concerning static methods... I usually place them before the others, but sometimes a method "staticity" changes and it's not worth to move it around (because of source control). It also leads to moving helper methods far from where they gets used.

Currently I'm seriously considering the chronological order.

\$\endgroup\$
2
  • \$\begingroup\$ Should inner class Builder be placed before class and instance members\methods? Or it doesn't matter? \$\endgroup\$ Commented Oct 5, 2014 at 17:40
  • \$\begingroup\$ @LeonidSemyonov I guess, everyone places inner classes at the top, but I haven't seen such a rule. I do it, too, but I place all variable at the bottom, as inner classes would basically place them in the middle of a long text and so make them hard to find. \$\endgroup\$
    – maaartinus
    Commented Oct 5, 2014 at 17:43
1
\$\begingroup\$

First of all I think this piece of code is breaking KISS design. For example why do you need Creator? It is increasing the complexity and does the same as WeatherForecastRequest(Parcel source), or if you really need it I think it's better if Creator has a createFromBuilder() method too.

Name builder() is not representing what it is doing. createBuilder() would be better.

Why is the Builder class public, or why do you need builder() at all? By making Builder public, the class complexity is growing, and it becomes harder to use it. If it's possible you should follow the "one operation in one way" thing. That class has a public modifier with only a private constructor?

private final List<NameValuePair> arguments;: BasicNameValuePair is immutable, so my suggestion is next to arguments is final could be wrap by Collections.unmodifiableList(). With this solution you don't need to copy arguments into a new List at getArguments().

The order of fields, methods, constructors does hurt my eyes. I suggest not to mix things, because it causes spaghetti code, and is hard to read. For example I couldn't see how many constructors this class has.

You have to merge if statements at equals(). It's more readable and less overhead.

for (int i = 0; i < numberOfArguments; ++i) is odd, foreach is a much better solution.

public class ForecastParameters is only used from a private constructor. Are you sure it must be a public class? I suggest to use enumerator which is a better solution to store data like ForecastParameters.

\$\endgroup\$
4
  • \$\begingroup\$ AFAIK builder() is the standard name for the method. I forgot to write that this code feels rather overengineered, good that you mention it. \$\endgroup\$
    – maaartinus
    Commented Oct 5, 2014 at 15:12
  • \$\begingroup\$ @mhmxs Effective Java by Joshua Bloch - Item 2: Consider a builder when faced with many constructor parameters. But @maaartinus is right - there is no need in Builder. @mhmxs what do you mean by You have to merge if statements at equals(). Which of if statements I could override? \$\endgroup\$ Commented Oct 5, 2014 at 17:47
  • \$\begingroup\$ if (obj == this) { return true; } else if ... \$\endgroup\$
    – mhmxs
    Commented Oct 5, 2014 at 19:42
  • \$\begingroup\$ @mhmxs there is one improvement in equals() method, yes. Instead of if ((obj == null) || !(obj instanceof MyClass)) { return false; } I can write if (!(obj instanceof MyClass)) { return false; } because the statement null instanceof SomeClass always returns false. \$\endgroup\$ Commented Oct 6, 2014 at 11:13

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.