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.

Anyone have any comments? Just trying to better my code if can be done.

public class SacWidget extends AppWidgetProvider {
    String roseUrl;
    AQuery aq;

    public void onUpdate(Context context, AppWidgetManager appWidgetManager,
            int[] appWidgetIds) {
        final int N = appWidgetIds.length;
        for (int i = 0; i < N; i++) {
            int appWidgetId = appWidgetIds[i];
            Intent intent = new Intent(context, DangerRose.class);
            PendingIntent pendingIntent = PendingIntent.getActivity(context, 0,
                    intent, 0);
            RemoteViews views = new RemoteViews(context.getPackageName(),
                    R.layout.widget_layout);
            views.setImageViewBitmap(R.id.ivdangerrose, getRose());
            appWidgetManager.updateAppWidget(appWidgetId, views);
        }
    }

    private Bitmap getRose() {
        Bitmap bitmap = null;
        File f = new File("/storage/emulated/0/acr/sac/dangerrose.png");
        try {
            bitmap = BitmapFactory.decodeStream(new FileInputStream(f));
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }

        return bitmap;
    }
}

EDIT... Made some changes via the comments...

public class SacWidget extends AppWidgetProvider {
    private static final String TAG = "SacWidget";

    public void onUpdate(Context context, AppWidgetManager appWidgetManager,
            int[] appWidgetIds) {
        for (int appWidgetId : appWidgetIds){
            File ext = Environment.getExternalStorageDirectory();
            File file = new File(ext,"acr/sac/dangerrose.png");
            Date lastModified = new Date(file.lastModified());

            String lm = "Updated: " + lastModified.toLocaleString();

            RemoteViews views = new RemoteViews(context.getPackageName(),
                    R.layout.widget_layout);

            Intent clickIntent = new Intent(context, ReportFullActivity.class);
            PendingIntent clickapp = PendingIntent.getActivity(context, 0,
                    clickIntent, PendingIntent.FLAG_UPDATE_CURRENT);

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

    private Bitmap getRose() {
        Bitmap bitmap = null;
        File ext = Environment.getExternalStorageDirectory();
        File dangerrosefile = new File(ext,"acr/sac/dangerrose.png");

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

        return bitmap;

    }
}
share|improve this question
add comment

2 Answers

up vote 3 down vote accepted

I'm not too familiar with Android, so the following is just some generic Java notes:

  1. Fields roseUrl and aq seem unused as well as the pendingIntent local variable. You might remove them.

  2. You could use a foreach loop:

    for (final int appWidgetId: appWidgetIds) ...
    
  3. It isn't the best idea to use printStackTrace() in Android exceptions.

  4. The getRose() method creates a stream (new FileInputStream(f)). I'm not sure whether BitmapFactory.decodeStream closes it before it returns or not. If not, you should close it.

Notes for the edit:

  1. You should close the stream in a finally block. If BitmapFactory.decodeStream throws an exception it won't be closed. See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

  2. The following two lines are duplicated:

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

    You could extract them out to a method.

share|improve this answer
1  
Thanks for the answer, I really appreciate it. I cleaned up the code a bit. I cannot find a close for the IS in any of the documentation.... hmmmm.... –  jasonflaherty Jan 31 '13 at 22:26
add comment
  1. Bad variable names. Please do not use things like N, i, and f (and if you are, at least be consistent and make them all lowercase). That aq is questionable as well.

  2. Using for (int appWidgetId : appWidgetIds) would save you a couple lines, and get rid of that atrocious i and N variables as well.

  3. Personally I wouldn't bother declaring N as final (or at all, since you could just as easily use appWidgetIds.length in your for loop condition even if you don't do #2) unless there is reason to. Like if you're going to refer to it from an anonymous class. Note that if you do #2 above, N goes away completely.

  4. Close your FileInputStream. Instead of doing File f = ... do FileInputStream input = new FileInputStream(new File("/storage/...")); and then input.close(); when you are done with it.

share|improve this answer
    
Thanks aroth! I updated the code with you and @palacsint comments. See the edit. –  jasonflaherty Feb 1 '13 at 2:25
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.