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;
    }
}
share|improve this question

2 Answers 2

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
  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

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.