Home screen widget to display random numbers

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 widgetsMap = null;

    public static MainAppClass getInstance() {
        return singleton;
    }

    @Override
    public void onCreate() {
        super.onCreate();
        singleton = this;
        widgetsMap = new HashMap();
    }

}

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 s = MainAppClass.getInstance().widgetsMap.
keySet();

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

TL;DR

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

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

• 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();
}

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

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

StackExchange Code Review Q#37488, answer score: 10