Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using Struts2 and Google App Engine. Below is my program for getting the country based on the IP of the visitor of the website.

In my Struts2 action:

HttpServletRequest request = ServletActionContext.getRequest();
String IP = request.getRemoteAddr();
String countryCode = Utilities.fetchUrl( "http://api.hostip.info/country.php?ip=" + IP );
if ( countryCode != null){
    String country = CountryUtil.COUNTRY_MAP.get( countryCode );
    System.out.println("COUNTRY: " + country);
}

My fetchUrl static function:

public static String fetchUrl(String strUrl){
    String output = "";
    String line = null;
    try {

        URL url = new URL( strUrl );
        BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream()));
        while ((line = reader.readLine()) != null) {
            output += line;
        }
        reader.close();

    } catch (MalformedURLException e) {
        System.out.println("ERROR CATCHED: " + e.getMessage());
        return null;
    } catch (IOException e) {
        System.out.println("ERROR CATCHED: " + e.getMessage());
        return null;
    }

    return output;
}

My COUNTRY_MAP:

public static final Map<String, String> COUNTRY_MAP;
static {
    Map<String, String> map = new HashMap<String, String>();
    map.put("AF", "afghanistan");
    map.put("AL", "albania");
    ...
    map.put("ZW", "zimbabwe");
    COUNTRY_MAP = Collections.unmodifiableMap( map );
}

Any suggestion on improving its speed? I also need suggestions for the coding.

share|improve this question
1  
Have a look in docs.oracle.com/javase/1.5.0/docs/api/java/util/EnumMap.html , and at Item 33 of Joshua Bloch's Extreme Java book –  cl-r Feb 25 '13 at 7:34
2  
Not related to speed: reader.close(); should be in a finally-block. –  MrSmith42 Feb 25 '13 at 10:57

2 Answers 2

The slowest part will be the request to the remote server. There is not much you can do to make this faster. You may want to cache the results you get back from hostip, or you may want to have a local database.

Furthermore, the API seems to return XX if it does not know the country, you may want to check for that.

Also, you do not close the InputStream or the InputStreamReader, and it is possible that you do not close the BufferedReader when an exception occurs. I don't know exactly whether this will cause any problems.

share|improve this answer

I would agree to the comment Answer from @Sjoerd. The slowest part is the http request. You could save some very little time to access this in other ways, but most probably, the packet transfer times are much higher than everything else.

Beside this, you should clean up your utility method and make it more robust.
I would suggest to create a separate class, the constructor takes the ip as an argument. The class has a read or updateData method and has getters like getCountryName(), getCountryCode(), getIp() and so one.
Then you could use one of this the json or the xml: http://api.hostip.info/get_json.php
http://api.hostip.info/?ip=
See http://www.hostip.info/use.html

And you have all information directly from the result. You should not create the map by youself, because you will easily forget something (my result was EU, you got this inside the map?) and you will never update it again.

Next step would be to handle your exceptions in a proper way. You should have finaly blocks as already suggested and you should handle errors. If you have your own update method, it could return a boolean indicating the success. And so one.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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