-3
\$\begingroup\$
    public class URLBuilder {

    StringBuffer api_url;
    final String AND = "&";
    private String API_KEY;
    String parameter;


    public void setAPI_KEY(String API_KEY) {

        this.API_KEY = API_KEY;
    }

    public void setCity_name(String cityname) {
        parameter = "q="+ cityname;

    }

    public void setCity_id(String cityId) {
        parameter = "id="+cityId;

    }

    public void setLatLon(double latitude, double longitude) {

        parameter = "lat="+latitude + AND + "lon="+longitude;

    }

    public void build() {

        api_url = new StringBuffer("http://api.openweathermap.org/data/2.5/weather?");
        api_url.append(parameter);
        api_url.append(AND);
        api_url.append("APPID="+API_KEY);

    }
}

public class TestClass {

    public static void main(String args[])
    {
        URLBuilder u = new URLBuilder();
        u.setAPI_KEY("af5f8fb56403d72444446fe7e097bef");
        u.setCity_name("LONDON");
        u.build();


    }

}

This is mycode to create a url for the openweathermap api. I am attempting to create a url builder class so the user can choose the parameters and the api key. I'd like to know how can i improve the code quality.

Also what should i do to call all the methods as below of the class.

u.setAPI_KEY("af5f8fb56403d72444446fe7e097bef").setCity_name("LONDON").build();
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This code doesn't generate a syntactically valid URL for "Mexico City". \$\endgroup\$ Nov 15, 2019 at 8:04

2 Answers 2

1
\$\begingroup\$

Only constants should be written uppercase (and might contain underscores). Other variable names should not contain underscores rather use camelcase (e.g. apiUrl).

Constants (in your case AND) should be declared final static, so Java only allocate memory once for the value (and not for every new object).

Use explicit access modifiers for your variables and not the default one. In your case: private.

The usage of blank lines in your setXXX methods are not consistent. I wouldn't use any blank lines at all in those methods but if you want to use them, use them consistently.

Don't use underscores in your method names.

Sometimes you surround the plus operator (+) with spaces, sometimes you don't. Do it consistently. Preferable surrounded by spaces.

setCity_id and setCity_name are no setters. They add something to a string. Either handle it like the setAPI_KEY method where you are storing the value in a variable which you later use to concatenate the string. Or rename the method to e.g. addCity_name to make it clear that there's a difference.

You are concatenating a string with the plus operator here:

api_url.append("APPID="+API_KEY);

That's inconsistent. API_KEY should be appended on its own. Beside that since Java 9 it's most of the times better to use the plus operator instead of a StringBuffer (https://dzone.com/articles/jdk-9jep-280-string-concatenations-will-never-be-t)

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

In addition to the comments by Joachim, I would suggest getting rid of the api_url member variable, and change your build() function to return a String.

I think that this class also needs some documenting comments to make it clear that you can have a city name, or a city ID, or lat/long, but if you try to use more than one, then the last one will overwrite the earlier ones.

\$\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.