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.
String slm = status.getHeader("last-modified");
        SimpleDateFormat sdf = new SimpleDateFormat(
                "EEE, dd MMM yyy HH:mm:ss zzz");

        Date serverLastMod = null;
        try {
            serverLastMod = sdf.parse(slm);
        } catch (ParseException e) {
            Log.e("SAC XML last-modified", e.getMessage());
        }
        Date fileLastFetched = status.getTime();

        Log.e("DATE1", serverLastMod.toGMTString());
        Log.e("DATE2", fileLastFetched.toGMTString());

        //if the server date is before my file date, update it, else dont

        if (serverLastMod.after(fileLastFetched)) {
            Log.e("DS", "YES! Getting ROSE");

            XmlDom r = xml.tag("reportrose");
            roseUrl = r.tag("img").attr("src").trim();
            aq.download(roseUrl, file, new AjaxCallback<File>() {

                public void callback(String url, File file, AjaxStatus status) {
                    String s = status.getMessage();
                    int i = status.getCode();
                    String e = s + " | Status Code: " + i;

                    if (file != null) {
                        getInfo();
                    } else {
                        Log.e("ACR SAC Widget Error", e);
                    }
                }

            });
        } else {
            Log.e("DS", "NO! Don't do anything... Wait until next check...");
        }
share|improve this question
add comment

2 Answers

Well, if it works, it works. There is not much to say about this small part of the code without any context. You should know this by yourself, because you should have some tests.


For the code:

The first catch is not suitable. If it happens, you will get a NullPointerException for the following if.


Avoid abbreviations. There are a lot of bad examples in this code, the most noticeable one is String e = s + " | Status Code: " + i;


Reduce indentation levels:

if (serverLastMod.after(fileLastFetched)) {
...
} else {
    Log.e("DS", "NO! Don't do anything... Wait until next check...");
}

could be:

if (!serverLastMod.after(fileLastFetched)) // or last <= current
{
    log...
    return
}
... rest of code...

and so on.
The part ...rest of code... could probably have its own method update..., so the purpose is clear.

share|improve this answer
add comment
String s = status.getMessage();
int i = status.getCode();
String e = s + " | Status Code: " + i;

can be directly rewrited :

String e = status.getMessage() + " | Status Code: " + status.getCode();

or more clean with Effective Java (Item 51)

StringBuilder sbTmp = new StringBuilder('MaxLenghtOfNormalMessages')
sbTmp.append(status.getMessage());
sbTmp.append( " | Status Code: ");
sbTmp.append(status.getCode());

Put also sbTmp in the else { .. } if it is not used outside it

share|improve this answer
1  
I do not agree with the last point (replacing with StringBuilder). The item in effective java from bloch (not extreme java) mentions string rebuilding inside loops with lots of iterations. For the given example, the + operator is absolutely fine. It is more readable and will most probably be replaced by the interpreter/compiler to a StringBuilder anyway. This is an example of bad premature optimization. –  tb- Mar 6 '13 at 16:00
    
@tb- I do not know why I always change the title of Joshua Bloch book's, corrected. Perhaps StringBuilder looks luxury, but the main impact is given by the size of messages expected. The more efficient is to put it in the else : only build if necessary. –  cl-r Mar 6 '13 at 16:07
add comment

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.