Android Widget Code Review

Question

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

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;

    }
}

palacsint · Accepted Answer · 2013-02-07 00:14:34Z

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

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

You could use a foreach loop:

for (final int appWidgetId: appWidgetIds) ...

It isn't the best idea to use printStackTrace() in Android exceptions.
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:

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

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.

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

aroth · Answer 2 · 2013-02-01 00:43:05Z

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.
Using for (int appWidgetId : appWidgetIds) would save you a couple lines, and get rid of that atrocious i and N variables as well.
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.
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.

Thanks aroth! I updated the code with you and @palacsint comments. See the edit.

asked	8 months ago
viewed	102 times
active	7 months ago

Android Widget Code Review

2 Answers

Your Answer

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

Android Widget Code Review

2 Answers

Your Answer

Sign up or login

Post as a guest

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

Related