Homescreen Widget Service

Question

I have implemented changes from a question I asked on this widget here:

Android widget code

I've added and changed a few things, so I would love some more coding advice!

public class SacWidgetService extends Service {
    private static final String TAG = "SAC WIDGET SERVICE";
    AQuery aq;
    private String roseXML = "sac-full.xml";
    private String roseUrl;
    private AppWidgetManager appWidgetManager;
    private int[] allWidgetIds;
    private File ext;
    private File file;

    @Override
    public void onStart(Intent intent, int startId) {

        aq = new AQuery(this);
        // Create some random data
        appWidgetManager = AppWidgetManager.getInstance(this
                    .getApplicationContext());

        allWidgetIds = intent.getIntArrayExtra(AppWidgetManager.EXTRA_APPWIDGET_IDS);

        // get external directory and put the file in there.
        ext = Environment.getExternalStorageDirectory();
        file = new File(ext, "avalancheconditionsreport/sac/dangerrose.png");

        // parse xml and get the rose
        xml_ajax();

        stopSelf();

        super.onStart(intent, startId);
    }

    @Override
    public IBinder onBind(Intent intent) {
        return null;
    }

    public void xml_ajax() {
        aq.ajax(roseXML, XmlDom.class, this, "sacCb");
    }

    public void sacCb(String url, XmlDom xml, AjaxStatus status) {
        XmlDom r = xml.tag("reportrose");
        roseUrl = r.tag("img").attr("src").trim();

        File ext = Environment.getExternalStorageDirectory();
        File target = new File(ext,
                "avalancheconditionsreport/sac/dangerrose.png");

        aq.download(roseUrl, target, 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);
                }
            }

        });
    }

    private Bitmap getRose() {
        Bitmap bitmap = null;
        try {
            FileInputStream input = new FileInputStream(file);
            bitmap = BitmapFactory.decodeStream(input);
            try {
                input.close();
            } catch (IOException e) {
                // TODO Auto-generated catch block
                Log.e(TAG, e.getMessage());
            }
        } catch (FileNotFoundException e) {
            Log.e(TAG, e.getMessage());
        }
        // Toast.makeText(this, "Sierra Avalanche Center Danger Rose",
        // Toast.LENGTH_LONG).show();
        return bitmap;

    }

    private void getInfo() {
        // TODO Auto-generated method stub
        Date dlm = new Date(file.lastModified());
        String lm = dlm.toLocaleString();

        for (int appWidgetId : allWidgetIds) {
            RemoteViews views = new RemoteViews(this.getApplicationContext()
                    .getPackageName(), R.layout.sac_widget_layout);

            Intent clickIntent = new Intent(this.getApplicationContext(),
                    SACActivity.class);
            PendingIntent clickapp = PendingIntent.getActivity(
                        this.getApplicationContext(), 0, clickIntent,
                        PendingIntent.FLAG_UPDATE_CURRENT);

            views.setOnClickPendingIntent(R.id.ivdangerrose, clickapp);
            views.setTextViewText(R.id.tvupdated, "Updated: " + lm);
            views.setImageViewBitmap(R.id.ivdangerrose, getRose());
            appWidgetManager.updateAppWidget(appWidgetId, views);
        }
    }        
}

Answer 1

1. try {
       FileInputStream input = new FileInputStream(file);
       bitmap = BitmapFactory.decodeStream(input);
       try {
           input.close();
       } catch (IOException e) {
           // TODO Auto-generated catch block
           Log.e(TAG, e.getMessage());
       }
   } catch (FileNotFoundException e) {
       Log.e(TAG, e.getMessage());
   }
   

   If decodeStream throws an exception the stream won't be closed. A proper solution is the following:

   try {
       FileInputStream input = new FileInputStream(file);
       try {
           bitmap = BitmapFactory.decodeStream(input);
       } finally {
           input.close();
       }
   } catch (FileNotFoundException e) {
       Log.e(TAG, e.getMessage());
   } catch (IOException e) {
       Log.e(TAG, e.getMessage());
   }
   

   Or this:

   try {
       FileInputStream input = new FileInputStream(file);
       try {
           bitmap = BitmapFactory.decodeStream(input);
       } finally {
           input.close();
       }
   } catch (IOException e) {
       Log.e(TAG, e.getMessage());
   }
   

   (Since FileNotFoundException is a subclass of IOException).

2. AQuery aq;
   

   I guess this field could be private. (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

3. private String roseUrl;
   ...
   private File ext;
   

   These field is used only in one method (onStart and sacCb). They could be local variables instead. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

4. public void xml_ajax() {
   

   This method name does not follow the usual camelCase conventions and it should be a verb. From Code Conventions for the Java Programming Language:

   Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

5. I'd use longer and more descriptive variable names (which explains their purpose) for better readability than this ones:

   XmlDom r = xml.tag("reportrose");
   ...
   String s = status.getMessage();
   int i = status.getCode();
   String e = s + " | Status Code: " + i;
   ...
   String lm = dlm.toLocaleString();
   

   Longer names would make the code more readable since readers don't have to decode the abbreviations every time and when they write/maintain the code don't have to guess which abbreviation the author uses.

6. private Bitmap getRose() {
       Bitmap bitmap = null;
       try {
           FileInputStream input = new FileInputStream(file);
           try {
               bitmap = BitmapFactory.decodeStream(input);
           } finally {
               input.close();
           }
       } catch (IOException e) {
           Log.e(TAG, e.getMessage());
       }
       return bitmap;
   }
   

   You can get rid of the bitmap variable here if you return immediately:

   private Bitmap getRose() {
       try {
           FileInputStream input = new FileInputStream(file);
           try {
               return BitmapFactory.decodeStream(input);
           } finally {
               input.close();
           }
       } catch (IOException e) {
           Log.e(TAG, e.getMessage());
       }
       return null;
   }
   

   See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

7. String lm = dlm.toLocaleString();
   

   This method is deprecated, you should use something else.

8. // TODO Auto-generated catch block
   

   Comments like this doesn't seem like professional work. Solve the issues and remove the comments.