Code Review Request for Alarm/Notificaiton part

[GoogleCodeExporter]

<Steps>

1. Update Alarm.java, ConfirmationPage.java, OneTimeAlarmReceiver.java, 
and AndroidManifest.xml to the newest ones.
2. Code review them.

Ok, I request a code review for those classes above. But, there are most 
likely be changed as I or other team memebers will add more features to 
it. Those classes are for the alarm/notification part.

Original issue reported on code.google.com by byonil0...@gmail.com on 19 Feb 2010 at 8:38

[GoogleCodeExporter]

CODE REVIEW - Bus Stop Alarm
Date: Feb 20, 2010
File: ConfirmationPage.java
Revision: 112
Author: Pyong Byon
Reviewer: Derek Cheng

Summary:

This is a first review of ConfirmationPage.java. This file contains the code 
for the 
confirmation page of the application, where the user selects the ringtone and 
other 
preferences. It works in conjunction with Alarm.java, 
OneTimeAlarmReceiver.java, and 
the manifest file to utilize the alarm/notification feature of Android. This 
review 
will examine codes that are written by Pyong; in particular, the methods that 
are 
responsible for creating a notification from user settings.  
This review will focus on code formatting, adherence to Java / Android style 
guidelines, logic, and object-oriented design principles.

Comments:                                                                       

CODING STANDARDS

There are some identation and spacing issues in onCreate().
There is a space missing in line 201 between "IOException" and the opening 
curly 
brace.
In line 342 you have an int variable named "index_prox"- seems like it is for 
debugging purposes only and is no longer used since you commented out the debug 
code. 
If you decide to keep it, you should rename it so it does not use underscores 
(e.g. 
indexProx).
Similar naming issues with "index_ringtone" and "ringtone_uri" around lines 
403-404. 
"ringtone_uri" appears in several other places to make sure to replace all of 
them.
It seems like there are several overlapping fields between ConfirmationPage and 
Alarm 
classes. If possible, you might want to move all of these information to Alarm 
so we 
won't have duplicate data. 

COMMENTS

Make sure to have comments on at the very top of the each file documenting the 
file's 
purpose and its author.
Use javadocs-style comments for documenting methods would make the code easier 
for 
others to read. For example, see Huy's BusDbAdapter.java.
There are quite a few lines which you had in the file for small testing 
purposes but 
they have now been commented out. If they are no longer needed, please remove 
them so 
the files will be easier to read.

LOGIC

In getVibrate(), if the lines you commented out from lines 283-292 are no long 
necessary, you can just say "vibration = isChecked;" to simplify the logic a 
little.

ERROR HANDLING

Is setSettings() guaranteed to be successful? If there is going to be some sort 
of 
validation of Strings involved in that method there might be potential for 
failure. 
Would it be a better idea to make it have a return type of boolean (indicating 
success / failure)?

CODE DECISIONS

The time field in ConfirmationPage class is declared as a static variable, but 
should 
it be the case that each isntance of ConfirmationPage should have its own timer?
Perhaps it is just because there are too many lines that you have commented 
out, but 
it seems the onCreate() method seems really long - would be great if you could 
take 
out something and make it a separate method.

Original comment by chen...@gmail.com on 21 Feb 2010 at 11:58

[GoogleCodeExporter]

Original comment by chen...@gmail.com on 23 Feb 2010 at 7:28

• Changed state: Done

[GoogleCodeExporter]

Ok, Thank you for code-reviewing.

These are what I changed after your code review.

- I wrote the author and basic description on top of confirmation page class 
and 
alarm class. 

- I deleted OneTimeAlarmReceiver class becuase I combined confirmation page 
with 
alarm and renamed OneTimeAlarmReceiver to Alarm class.

- I commneted on top of each method describing what the methods are doing, 
accepting 
as parameters, and returning.

- I deleted unnecessary lines of code (mostly those previously needed for 
debugging)

- I changed variable names to meet the java code style. (for example, changed 
data_vibrate to dataVibrate)

- fixed: vibration = isChecked, (previously it was a bit redundant).

- I deleted setSettings() method because I changed codes and I didn't need it 
anymore.

Original comment by byonil0...@gmail.com on 24 Feb 2010 at 7:30