5
\$\begingroup\$

I found a example of Java PostMethod here.

But I want to post several times by using for loop. I made it like this.


import org.apache.commons.httpclient.HttpClient;
import org.apache.commons.httpclient.HttpStatus;
import org.apache.commons.httpclient.methods.PostMethod;

import java.io.BufferedReader;
import java.io.InputStreamReader;

public class PostMethodExample {

  public static void main(String args[]) {

    HttpClient client = new HttpClient();
    client.getParams().setParameter("http.useragent", "Test Client");

    BufferedReader br = null;

    PostMethod method = new PostMethod("http://search.yahoo.com/search");
    method.addParameter("p", "\"java2s\"");

    try{
            String[] parameters = { "Google", "Yahoo", "MSN" };

            for (String s in parameters){
                int returnCode = client.executeMethod(method);
                method.addParameter("p", s);
                if(returnCode == HttpStatus.SC_NOT_IMPLEMENTED) {
                    System.err.println("The Post method is not implemented by this URI");
                    // still consume the response body
                    method.getResponseBodyAsString();
                } else {
                    br = new BufferedReader(new InputStreamReader(method.getResponseBodyAsStream()));
                    String readLine;
                    while(((readLine = br.readLine()) != null)) {
                        System.err.println(readLine);
                    }
                }
                method.releaseConnection();
                method = new PostMethod("http://search.yahoo.com/search");
            }
    } catch (Exception e) {
      System.err.println(e);
    } finally {
      method.releaseConnection();
      if(br != null) try { br.close(); } catch (Exception fe) {}
    }
  }
}

I modified code just inside of try block. I don't think this is the best way. Help me to improve this code.

\$\endgroup\$
  • 4
    \$\begingroup\$ for (String s in parameters)? Is it supposed to compile? And don't swallow exceptions, report them. \$\endgroup\$ – PhiLho Jun 7 '11 at 18:19
  • 4
    \$\begingroup\$ While System.err.println(e) isn't really swallowing an exception, it's not really reporting it properly either. If you must print somewhere instead of logging, at least use e.printStackTrace() \$\endgroup\$ – Alan Krueger Mar 19 '12 at 20:58
  • \$\begingroup\$ I don't think you want to modify the inside of the try block, as you've effectively prevented the finally block from releasing each connection you're attempting. \$\endgroup\$ – Alan Krueger Mar 19 '12 at 21:03
6
\$\begingroup\$

Avoid catching all exceptions

Do not use catch (Exception e) as this will also catch NullPointerException, ArrayIndexOutOfBoundsException and a whole lot of other ones. Only catch the Exceptions you need to catch, such as IOException. It is said that one should be specific in throws clause, but that applies for catch as well.

And as stated in the comments to your question, System.err.println(e) does not give full information about the exception. If you don't have a logger system nearby that can handle exceptions completely, at least use e.printStackTrace();. It is however recommended that you should handle and exception, and not only report it. For example by displaying a more accurate error message to the user.

Also, I would say that your line if(br != null) try { br.close(); } catch (Exception fe) {} should be on multiple lines. Even if it increases line count, it improves readability.

if (br != null) {
    try {
       br.close();
    }
    catch (Exception fe) {
    }
}
\$\endgroup\$
5
\$\begingroup\$

I think this pattern of creating an instance, running it through the loop, and at the end of the loop re-initializing it, is a bad one. This case is a good example of what can go wrong... because you have introduced a bug as a result

The first time through the loop your method is configured as:

PostMethod method = new PostMethod("http://search.yahoo.com/search");
method.addParameter("p", "\"java2s\"");

but for the remaining loop iterations the method is simply:

method = new PostMethod("http://search.yahoo.com/search");

Similarly, another bug that has 'creeped' (crept?) in is you change the 'p' parameter after the request is posted....

int returnCode = client.executeMethod(method);
method.addParameter("p", s);

There is no reason, in this case, why you can't initialize the method inside the loop, it makes more sense, and you eliminate bugs....:

for (String s in parameters){

    final PostMethod method = new PostMethod("http://search.yahoo.com/search");
    method.addParameter("p", s);
    int returnCode = client.executeMethod(method);

    ....

    method.releaseConnection();
 }
\$\endgroup\$
2
\$\begingroup\$

I find your indentation a bit strange. You have two-space indentation in one place, and eight-space in another. Keep it consistent.

Java conventions recommend four-space indents.

public class PostMethodExample {

    public static void main(String args[]) {
        HttpClient client = new HttpClient();
        client.getParams().setParameter("http.useragent", "Test Client");

        BufferedReader br = null;

        PostMethod method = new PostMethod("http://search.yahoo.com/search");
        method.addParameter("p", "\"java2s\"");

        try{
            String[] parameters = { "Google", "Yahoo", "MSN" };

            for (String s in parameters){
                int returnCode = client.executeMethod(method);
                method.addParameter("p", s);
                if(returnCode == HttpStatus.SC_NOT_IMPLEMENTED) {
                    System.err.println("The Post method is not implemented by this URI");
                    // still consume the response body
                    method.getResponseBodyAsString();
                } else {
                    br = new BufferedReader(new InputStreamReader(method.getResponseBodyAsStream()));
                    String readLine;
                    while(((readLine = br.readLine()) != null)) {
                        System.err.println(readLine);
                    }
                }
                method.releaseConnection();
                method = new PostMethod("http://search.yahoo.com/search");
            }
        } catch (Exception e) {
            System.err.println(e);
        } finally {
            method.releaseConnection();
            if(br != null) try { br.close(); } catch (Exception fe) {}
        }
    }
}

Also, you have places where you do:

try{

and places where you do:

if(returnCode == HttpStatus.SC_NOT_IMPLEMENTED) {

Again, consistency. Always put a space before the opening brace, or never. I recommend always, as that is more readable.


And as it was mentioned in a comment:

for (String s in parameters)? Is it supposed to compile?

I don't think Java supports this kind of statement. Have you tested the code?

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