8
\$\begingroup\$

I have created a small HomeScreen Widget which displays random numbers. Attached is the code. Please let me know if the approach is good.

https://github.com/gottp/HomeWidgetTest

MainAppClass.java

package com.example.homewidgettest;

import java.util.HashMap;

import android.app.Application;

public class MainAppClass extends Application{

    private static MainAppClass singleton;

    /* Maps to hold widget ids */
    public HashMap<Integer, Boolean> widgetsMap = null;

    public static MainAppClass getInstance() {
        return singleton;
    }

    @Override
    public void onCreate() {
        super.onCreate();
        singleton = this;
        widgetsMap = new HashMap<Integer, Boolean>();
    }

}

MyWidgetUpdateService.java

package com.example.homewidgettest;

import java.util.Random;
import java.util.Set;

import android.app.Service;
import android.appwidget.AppWidgetManager;
import android.content.ComponentName;
import android.content.Intent;
import android.os.Handler;
import android.os.IBinder;
import android.widget.RemoteViews;

public class MyWidgetUpdateService extends Service {
    Handler h = null;

    @Override
    public IBinder onBind(Intent arg0) {
        // TODO Auto-generated method stub
        return null;
    }

    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {
        if (h == null) {
            h = new Handler();
            h.postDelayed(widgetRunnable, 500);
        }
        return START_STICKY;
    }

    @Override
    public void onCreate() {
    }

    @Override
    public void onDestroy() {
    }

    private Runnable widgetRunnable = new Runnable() {
        public void run() {
            updateWidgets();
            h.postDelayed(widgetRunnable, 2000);
        }
    };

    void updateWidgets() {
        AppWidgetManager appWidgetManager = AppWidgetManager.getInstance(this
                .getApplicationContext());
        Set<Integer> s = MainAppClass.getInstance().widgetsMap.
                            keySet();
         Integer []wids = s.toArray(new Integer[s.size()]);

        for (int widgetId : wids) {
            // create some random data
            int number = (new Random().nextInt(100));

            RemoteViews remoteViews = new RemoteViews(this
                    .getApplicationContext().getPackageName(),
                    R.layout.widget_layout);

            // Set the text
            remoteViews.setTextViewText(R.id.update,
                    "Random: " + String.valueOf(number));

            appWidgetManager.updateAppWidget(widgetId, remoteViews);
        }

    }
}

MyWidgetProvider.java

package com.example.homewidgettest;

import android.appwidget.AppWidgetManager;
import android.appwidget.AppWidgetProvider;
import android.content.Context;
import android.content.Intent;
import android.util.Log;

public class MyWidgetProvider extends AppWidgetProvider {
    @Override
    public void onUpdate(Context context, AppWidgetManager appWidgetManager,
            int[] appWidgetIds) {
        try {
            MainAppClass myapp = MainAppClass.getInstance();
            for (int wid : appWidgetIds) {
                Integer i = wid;
                Log.e("AAA", String.valueOf(i) + "" + myapp.toString() + " " +
                        myapp.widgetsMap.toString());
                myapp.widgetsMap.put(wid, true);
            }
            if (appWidgetIds.length > 0) {
                /* Start a service */
                Intent intent = new Intent(context.getApplicationContext(),
                        MyWidgetUpdateService.class);
                context.startService(intent);
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    @Override
    public void onDeleted(Context context, int[] appWidgetIds) {
        try {
            MainAppClass myapp = MainAppClass.getInstance();
            for (int wid : appWidgetIds) {
                Integer i = wid;
                if (myapp.widgetsMap.containsKey(i)) {
                    myapp.widgetsMap.put(i, false);
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

My questions are:

  1. Do you find the implementation ok?
  2. I have used post Delayed instead of AlarmManager. Is this fine?
\$\endgroup\$
10
\$\begingroup\$

Looks like some nice Java! I just have a few comments. (Better late than never? Haha)

TL;DR

  1. Better variable names. Things like h, s, and i don't make for good variable names. Try to be descriptive and to name them for what they actually represent, like widgetMapKeys for your Set. This will make your code much easier to read, and thus more maintainable. (I promise, even if it's just you revisiting your own code six months later, you'll be grateful to yourself.)
  2. Better and consistent line breaks. The way you break up long lines into multiple is kind of awkward and muddies up your code. When dealing with this issue, try to perform line breaks where you can keep logical units together rather than arbitrarily throwing them in. Again, this has to do with code readability and maintainability.
  3. Always encapsulate fields. If you want to be object-oriented, class fields should be private and accessed through getters and setters. (An exception might be made when using inheritance, but this is a separate concept that has nothing to do with your project.)

Specific Issues

@Override
public void onCreate() {
    super.onCreate();
    singleton = this;
    widgetsMap = new HashMap<Integer, Boolean>();
}

If you run FindBugs on this, it will generate a warning about assigning a value to a static variable from an instance method. This is a good warning, and it's even more appropriate for a class which is supposed to be a singleton. Using this technique of assigning the instance variable, you must be absolutely sure that onCreate() will be called once and only once. Otherwise, your class isn't really a singleton, and any logic you've created which relies on it being one is subject to bugs.

/* Maps to hold widget ids */
public HashMap<Integer, Boolean> widgetsMap = null;

You say this is to hold IDs, but what does the Boolean represent? I'm confused why you can't just use a List<Integer>.

public class MyWidgetUpdateService extends Service {

This is a bit nitpick-y, but I really dislike classes which are named "MyWhatever". Try to be more descriptive. In this case WidgetUpdateService might even be sufficient.

Handler h = null;

Same thing here. Try to be more descriptive with your variable names.

void updateWidgets() {

You should always explicitly declare an access modifier (public, private, or protected) for your methods. (As @Marc-Andre points out in the comments below, there actually is a specific difference between protected and default access. You can read an explanation of the difference on this StackOverflow question. I'm not convinced there's a really good use-case for this, but there you go.)

AppWidgetManager appWidgetManager = AppWidgetManager.getInstance(this
        .getApplicationContext());
Set<Integer> s = MainAppClass.getInstance().widgetsMap.
                    keySet();

This bit of code is kind of muddy. I get that you're trying to not have the lines stretch out to the right, but there are some better formatting techniques you can use. Also, you should be consistent; on the first line, you break before the . and the method call, but on the second line you break after the .. Whatever you choose to do, at least do it the same way every time.

I'd probably rewrite it like this:

AppWidgetManager appWidgetManager = AppWidgetManager.getInstance(
        this.getApplicationContext()
);

Set<Integer> s = MainAppClass.getInstance().widgetsMap.keySet();

(You actually don't even need to call getApplicationContext() via this, but I kept it to remain consistent with your code.)

There's no need to even wrap the second line of code, since it's approximately the same length as the first (before the line break). Again, I'd also rename your s variable to something more meaningful, like widgetsMapKeys or something. It's also just my particular style to put the closing bracket and semicolon on a separate line when I do this (to me it just looks much cleaner and groups things nicely), but if you hate that you can just end the last indented line with it and still be okay stylistically.

These lines also have one other issue. The widgetsMap variable is accessed directly rather than through a getter and setter. If you're trying to be strictly object-oriented, you should always keep your class's fields private and encapsulate them (e.g., getWidgetsMap()).

Integer []wids = s.toArray(new Integer[s.size()]);

This line is a bit awkward, mostly because of the placement of []. You should really either have Integer[] wids or Integer wids[]. I strongly prefer the first one, but arguments can be made either way.

for (int widgetId : wids) {
    // create some random data
    int number = (new Random().nextInt(100));

Why are you creating a new Random() every time you pass through this loop? The only conceivable reason is if you think getting a new seed every time will increase your randomness, but that's not really true. All seeds have an approximately uniform distribution of random numbers. You should really have your Random as a constant, i.e., private static final Random RNG = new Random().

RemoteViews remoteViews = new RemoteViews(this
        .getApplicationContext().getPackageName(),
        R.layout.widget_layout);

Again, this has odd line breaks. I might rewrite it as something like the following:

RemoteViews remoteViews = new RemoteViews(
        this.getApplicationContext().getPackageName(),
        R.layout.widget_layout
);

Also, in this bit of code, what's up with R.layout.widget_layout? I'm not intimately familiar with Android API, but if this is something you coded, you should maintain consistent variable names (widgetLayout versus widget_layout) and encapsulation R.getLayout().getWidgetLayout()).

public class MyWidgetProvider extends AppWidgetProvider {
    @Override
    public void onUpdate(Context context, AppWidgetManager appWidgetManager,
            int[] appWidgetIds) {
        try {
            MainAppClass myapp = MainAppClass.getInstance();

Same comments here as above about using MyWhatever for class and variable names. Also, keep variables in camel case (myApp).

catch (Exception e) {
    e.printStackTrace();
}

Can you perform some actual logging here rather than just printing the stack trace to standard out? Also be aware that you're catching all exceptions with this, including runtime exceptions like NullPointerExceptions, which, in general, should not be caught. It might make sense for a UI to do so, however.

@Override
public void onDeleted(Context context, int[] appWidgetIds) {
    try {
        MainAppClass myapp = MainAppClass.getInstance();
        for (int wid : appWidgetIds) {
            Integer i = wid;
            if (myapp.widgetsMap.containsKey(i)) {
                myapp.widgetsMap.put(i, false);
            }
        }
    } catch (Exception e) {
        e.printStackTrace();
    }
}

I'm still a little confused as to why you're using a Map<Integer, Boolean> to store IDs rather than a simple List<Integer>. Is there some reason you want to hold on to IDs after they've been deleted? You could simply remove the ID from the List on delete, if not. Just trying to understand, because the simpler you can make your code, the better.

\$\endgroup\$
  • \$\begingroup\$ You should always explicitly declare an access modifier (public, private, or protected) for your methods. not excatly true, sometime package is visibility could be good, but I agree those are rare exceptions. \$\endgroup\$ – Marc-Andre Apr 8 '14 at 15:14
  • \$\begingroup\$ @Marc-Andre Do you really mean visibility only to the class's package? If so, protected serves that purpose, no? (Obviously this would also expose it to any classes which extend it as well.) \$\endgroup\$ – asteri Apr 8 '14 at 15:16
  • \$\begingroup\$ @Marc-Andre Ah, I guess that is the difference. Makes sense. I'll edit it into the post. Though I'm not sure of any good use-case for it. \$\endgroup\$ – asteri Apr 8 '14 at 15:19
  • \$\begingroup\$ No I'm talking about the default one, package visibility, where the method is private to everything except all classes in the same package. A good use-case has been for tests(not sure anymore that this is still the case with all the mock framework). \$\endgroup\$ – Marc-Andre Apr 8 '14 at 15:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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