6
\$\begingroup\$

So I'm very much a beginner to Java and Android programming. I've worked a lot more with Python so I might have some quirks from there bleeding into my code here. I'd like a particular focus on pointing out ways I'm approaching the language and any confusing style I might be employing.

What the app does is pretty simple. The user enters a task and then the app will regularly notify them a reminder to do the task if they haven't already. When the task is done, the user can enter the app, clear the task and set a new one. There's just one screen that has an EditText and two buttons, one for submitting a task and one for clearing it.

I'm not planning to expand it a lot, the intention is to be simple. But I will add settings, to change how long between notifications and whether notifications should have sounds or vibrate.

In my testing it seems that the timing of notifications is erratic, but the documentation says AlarmManagers are just like that. Should I trust this since I don't need a precise interval anyway?

Here's the full code, my MainActivity as well as my BroadcastReceiver and Service.

MainActivity

public class MainActivity extends AppCompatActivity {

    private EditText enterTask;
    private Button button;
    private Button clear_task;
    private String task;
    private SharedPreferences prefs;
    private final long SIX_HOURS = 21600000L;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        prefs = PreferenceManager.getDefaultSharedPreferences(this);
        enterTask = (EditText) findViewById(R.id.enter_task);
        button = (Button) findViewById(R.id.submit);
        clear_task = (Button) findViewById(R.id.clear_task);

        // Try get the current task String.
        task = (prefs.getString("Current_Task", ""));
        if (!task.equals("")) {
            Toast.makeText(getApplicationContext(),
                    "Current task is " + task,
                    Toast.LENGTH_LONG).show();
        }

        button.setOnClickListener(new View.OnClickListener() {
            public void onClick(View v) {
                if (!task.equals("")) {
                    Toast.makeText(getApplicationContext(),
                            "Task " + task + " is already set!",
                            Toast.LENGTH_LONG).show();
                } else {
                    task = enterTask.getText().toString();
                    if (task.equals("Enter task here...") || task.equals("")) {
                        Toast.makeText(getApplicationContext(),
                                "Please enter a task.",
                                Toast.LENGTH_LONG).show();
                    }
                    Toast.makeText(getApplicationContext(),
                            "Saved task " + task,
                            Toast.LENGTH_LONG).show();

                    SharedPreferences.Editor editor = prefs.edit();
                    editor.putString("Current_Task", task);
                    editor.apply();

                    Context context = MainActivity.this;
                    AlarmManager mgr = (AlarmManager) context.getSystemService(Context.ALARM_SERVICE);

                    Intent i = new Intent(context, Notify.class);
                    PendingIntent pi = PendingIntent.getBroadcast(context, 0, i, 0);

                    mgr.setRepeating(AlarmManager.ELAPSED_REALTIME_WAKEUP,
                            SystemClock.elapsedRealtime(),
                            SIX_HOURS,
                            pi);
                }
            }
        });

        clear_task.setOnClickListener(new View.OnClickListener() {
            public void onClick(View v) {
                if (task.equals("")){
                    Toast.makeText(getApplicationContext(),
                            "No task to clear!", Toast.LENGTH_LONG).show();
                } else {
                    SharedPreferences.Editor editor = prefs.edit();
                    editor.putString("Current_Task", "");
                    editor.apply();
                    task = "";

                    Toast.makeText(getApplicationContext(),
                            "Task cleared", Toast.LENGTH_LONG).show();
                }
            }
        });
    }
}

Notify

public class Notify extends BroadcastReceiver {
    public Notify() {
    }

    @Override
    public void onReceive(Context context, Intent intent) {
        Intent service1 = new Intent(context, NotifyAlarm.class);
        context.startService(service1);
    }
}

NotifyAlarm

public class NotifyAlarm extends Service {

    private SharedPreferences prefs;
    private String task;
    private NotificationManager mNotificationManager;
    private android.support.v4.app.NotificationCompat.Builder mBuilder;

    public NotifyAlarm() {
    }

    @Override
    public IBinder onBind(Intent intent) {
        return null;
    }

    @SuppressWarnings("static-access")
    @Override
    public void onStart(Intent intent, int startId)
    {
        super.onStart(intent, startId);

        prefs = PreferenceManager.getDefaultSharedPreferences(this);
        task = prefs.getString("Current_Task", "");

        if (!task.equals("")){
            mNotificationManager = (NotificationManager) getSystemService(Context.NOTIFICATION_SERVICE);

            Intent intent1 = new Intent(this.getApplicationContext(),MainActivity.class);
            mBuilder = new NotificationCompat.Builder(this)
                    .setSmallIcon(R.mipmap.ic_launcher)
                    .setContentTitle("Current task")
                    .setContentText("Hey! Have you " + task + "?");

            mNotificationManager.notify(0, mBuilder.build());
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Variables that can be local

All member variables in NotifyAlarm are only used in one method. As such, they can all be converted to local variables. It's recommended to define variables in the smallest scope necessary. In case of member variables, you need to read the entire class to understand all their uses.

The same is true for MainActivity, except the SIX_HOURS constant, and task, which can be left where they are.

Empty string?

Instead of !task.equals(""), the idiomatic way to check if a string is non-empty is using !task.isEmpty().

Empty constructors

I'm wondering if you really need the empty constructors, for example in Notify and NotifyAlarm. If they are not required, I suggest to remove them.

Naming

In Java, variables should be named camelCase, which is violated by clear_task. It's especially odd that it's inconsistent with enterTask, just a few lines away.

In Android development it's a common practice to prefix member fields with m. I'm not a big fan of that, but it's ok if you like it. However, either use it consistently everywhere, or use camelCase consistently everywhere.

i is a very poor name for an Intent, and pi for a PendingIndent. I suggest to just spell these out. Other variables could also have better names.

SIX_HOURS is literally what it means. It would be better to name it according to the purpose it represents.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the quick suggestions! I had somewhere heard that declaring variables at the top level of a Java class was best. I either misunderstood or perhaps they were saying that as beginner wisdom, to avoid explaining how scope works yet. Also I named SIX_HOURS such because my intention is to have multiple periods of time for the gap. Do you still think names like SHORT_INTERVAL would be better? \$\endgroup\$ – SuperBiasedMan Sep 12 '15 at 22:54
  • 1
    \$\begingroup\$ Constants are best at the top level. Everything else is best at the smallest scope necessary. Not only in Java, that's pretty universal in modern languages. Yes, SHORT_INTERVAL is better. When later you decide to change 6 hours to 4 hours, the variable name can stay, you can tweak the value and the code will still make sense. \$\endgroup\$ – janos Sep 12 '15 at 22:59
  • \$\begingroup\$ I'm making the changes now and I should say that I can't move task to be a local variable. It has to be set as final if it's moved inside the method (since it's being accessed from within both OnClick methods) but then that means it has to be declared as a constant string, rather than one that updates. So I left task outside with the constants and moved the rest in. \$\endgroup\$ – SuperBiasedMan Sep 20 '15 at 12:32
  • \$\begingroup\$ Good point, updated my answer accordingly, thanks! \$\endgroup\$ – janos Sep 20 '15 at 12:54

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.