1
\$\begingroup\$

I am creating a utility class to do an HttpURLConnection. I want to make this class generic as in I pass the URL and the POST params. It works with any range of these two values.

public class HttpUrlConnection {

    String url, params;

    public HttpUrlConnection(String url, String params) {
        this.url = url;
        this.params = params;

        try {
            sendPost();
        }catch(Exception e){
            System.out.println("Exception"+e);
        }
    }


    private String sendPost() throws Exception {

        String USER_AGENT = "Mozilla/5.0";
        URL obj = new URL(url);

        HttpURLConnection con = (HttpURLConnection) obj.openConnection();

        //add reuqest header
        con.setRequestMethod("POST");
        con.setRequestProperty("User-Agent", USER_AGENT);
        con.setRequestProperty("Content-Type", "application/x-www-form-urlencoded");

        String urlParameters = params;
        // Send post request
        con.setDoOutput(true);
        DataOutputStream wr = new DataOutputStream(con.getOutputStream());
        wr.writeBytes(urlParameters);
        wr.flush();
        wr.close();

        int responseCode = con.getResponseCode();
        System.out.println("\nSending 'POST' request to URL : " + url);
        System.out.println("Post parameters : " + urlParameters);
        System.out.println("Response Code : " + responseCode);

        BufferedReader in = new BufferedReader(
                new InputStreamReader(con.getInputStream()));
        String inputLine;
        StringBuffer response = new StringBuffer();

        while ((inputLine = in.readLine()) != null) {
            response.append(inputLine);
        }
        in.close();

        //print result
        System.out.println("rvsp" + response.toString());

        return response.toString();
    }


}

I am unsure if I am following the best practice above.

Should I keep the cosntructor short as:

public HttpUrlConnection(String url, String params) {
        this.url = url;
        this.params = params;
    }

and start the POST using connection.sendPost where in some other class I do:

HttpUrlConnection connection;
connection = new HttpUrlConnection(url,params);
connection.sendPost();
\$\endgroup\$
3
\$\begingroup\$
String url, params;

They should be private and final and each declared on its own line. However... they just get passed from the constructor to the method.

There's a single use. So you can drop them and pass them directly.

The next step is to drop the object and use a static method.

}catch(Exception e){
        System.out.println("Exception"+e);
    }

If eating exceptions was such a good idea as many think, no JDK method would throw anything. Imagine openConnection printing something like "Heghlu’meH QaQ jajvam" or "Connection reset by peer" to standard output and everyone happily looking at the console to tell their program what to do.

At the very very least, use e.printStackTrace. But the simplest solution is nearly always the best: Declare you method to throw and let it be!

sendPost();

You're throwing away the returned value.

In case of an exception you're leaking resources, use try-with-resources or alike to ensure everything gets closed.

Utility class

A typical utility class is java.lang.Math:

  • private constructor
  • no instances
  • no fields but static final constants
\$\endgroup\$
  • \$\begingroup\$ I will follow that, however declaring each variable on their own line is just for the sake of readability? Or am I missing something? \$\endgroup\$ – User3 May 20 '15 at 8:35
  • 1
    \$\begingroup\$ @User3 It's convention, which probably helps finding the variables fast. I personally don't really care, but actually follow it, as the number of potentially saved lines is not worth it. \$\endgroup\$ – maaartinus May 20 '15 at 8:53
  • \$\begingroup\$ @maaartinus hey, I also have question on Apache Http here. Wanted to see if you can help out. I already opened a bounty and that got finished as well without any luck. \$\endgroup\$ – user1950349 May 25 '16 at 0:13
4
\$\begingroup\$

If you would pass in your paramaters at sendPost(), you could reuse your created object for other URLs and Parameters, and would not need a specific constructor.

Generally, calling a Method in a constructor is defying the purpose of a constructor - to initialize an object. When rereading your code 2 months later, you would be wondering, where you'd be calling the actual POST.

Clean Code wise, it's generally a bad sign if you create subsections in your method using comments, such as //add reuqest header. Create seperate private methods in your class just for doing that. Your code will be more readable and reusable.

Exceptions; use them! Throw detailed Exceptions and declare them as such. So you can react in your other code to that exception.

String USER_AGENT = "Mozilla/5.0"; this should be either declared as private static final in your class, because when introducing other methods such as sendGet you'd be using the same variable again.

Generally, don't use System.out.println other than in very early stages of your code. Create a Response class, where you set the things such as the response code or others and return this, don't just write it to the console, you can't use it and your console is going to be littered with such output everywhere in your program, which makes it difficult to debug.

You could generalize the method creating your params String as well, so you could pass in an arbirtrary amount of parameters and it would serialize these parameters just right.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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