2
\$\begingroup\$

This is a simple app for taking down a user's home and work address, the days of their commute, and the times they go to work and go home. The times and workweek are entered via TimePicker dialogs with additional buttons. An IntentService runs in the background with setRepeating alarms that call per the user's set times, and there is a Broadcast Receiver that resumes the service upon device reboot. When an Alarm goes off, it asks the user if they'd like to check their commute. If the user taps on the notification, it runs a pre-formatted Google Maps search.

I'm an entry-level developer who is trying to improve himself and was hoping a code review can point out some inefficiencies or places where refactoring could be used. Code for the two classes I spent the most time in is below. If any editing improvements should be made for the benefit of reviews, please let me know and I'll clean up/clarify wherever it's requested.

public class MainFragment extends Fragment {
Button mWorkCommuteTimeButton;
Button mHomeCommuteTimeButton;
TextView mDisplayWorkCommute;
TextView mDisplayHomeCommute;
Button mSaveCommuteInfoButton;
EditText mEditWorkAddressText;
EditText mEditHomeAddressText;
static final String USER_INFO_FILE = "user_info.txt";
UserData savedUserInfo;
UserData mUser;

private static final String DIALOG_WORK_COMMUTE = "to work";
private static final String DIALOG_HOME_COMMUTE = "to home";
private static final String MAIN_MENU = "main menu";
private static final int REQUEST_WORK_COMMUTE=0;
private static final int REQUEST_HOME_COMMUTE=1;



@Override
public void onCreate(Bundle savedInstanceState)

{
super.onCreate(savedInstanceState);
setRetainInstance(true);
setHasOptionsMenu(true);
mUser = new UserData();
}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)

{
View v = inflater.inflate(R.layout.fragment_main, container, false);
mDisplayWorkCommute = (TextView)v.findViewById(R.id.userWorkDateTimeTextView);
mDisplayHomeCommute = (TextView)v.findViewById(R.id.userHomeTimeTextView);
mEditWorkAddressText = (EditText)v.findViewById(R.id.editWorkAddressTextView);
mEditHomeAddressText = (EditText)v.findViewById(R.id.editHomeAddressTextView);
mWorkCommuteTimeButton= (Button) v.findViewById(R.id.enterWorkDateTimeButton);

//always attempt to find saved user data to load first
try 
{
loadSavedInfo(USER_INFO_FILE);

{
mUser.copyUserData(savedUserInfo);
SimpleDateFormat sdf = new SimpleDateFormat("hh:mm aa ZZZZ", Locale.getDefault());
mDisplayWorkCommute.setText((sdf.format(mUser.mDriveToWorkTime.getTime()) + " " +mUser.workWeekString()));
mDisplayHomeCommute.setText((sdf.format(mUser.mDriveToHomeTime.getTime())));
mEditWorkAddressText.setText(mUser.getWorkAddress());
mEditHomeAddressText.setText(mUser.getHomeAddress());
}

}

catch (StreamCorruptedException e1)
{
// TODO Auto-generated catch block
e1.printStackTrace();
}
catch (ClassNotFoundException e1)
{
// TODO Auto-generated catch block
e1.printStackTrace();
}
catch (IOException e1)
{
// TODO Auto-generated catch block
e1.printStackTrace();
}

mWorkCommuteTimeButton.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
FragmentManager fm = getActivity().getSupportFragmentManager();
WorkCommuteDayAndTimeFragment dialog = WorkCommuteDayAndTimeFragment.newInstance(new UserData());
dialog.setTargetFragment(MainFragment.this, REQUEST_WORK_COMMUTE);
dialog.show(fm, DIALOG_WORK_COMMUTE);
}
});

mHomeCommuteTimeButton= (Button) v.findViewById(R.id.enterHomeTimeButton);
mHomeCommuteTimeButton.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
FragmentManager fm = getActivity().getSupportFragmentManager();
HomeCommuteDayAndTimeFragment dialog = HomeCommuteDayAndTimeFragment.newInstance(new UserData());
dialog.setTargetFragment(MainFragment.this, REQUEST_HOME_COMMUTE);
dialog.show(fm, DIALOG_HOME_COMMUTE);
}
});

mSaveCommuteInfoButton = (Button) v.findViewById(R.id.saveCommuteInfoButton);
mSaveCommuteInfoButton.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
mUser.setHomeAddress(mEditHomeAddressText.getText().toString());
mUser.setWorkAddress(mEditWorkAddressText.getText().toString());
try
{
saveInfo(mUser);
}
catch (IOException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
}
});
return v;
}

public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) {
super.onCreateOptionsMenu(menu, inflater);
inflater.inflate(R.menu.main_actions, menu);
}

@Override
public void onPrepareOptionsMenu(Menu menu)
{
super.onPrepareOptionsMenu(menu);
MenuItem toggleItem = menu.findItem(R.id.action_alarm_toggle);
getActivity().invalidateOptionsMenu();

if (CommuteCheckAlarmService.isServiceAlarmOn(getActivity()))
{
toggleItem.setTitle(R.string.action_turn_checker_off);
}
else
{
toggleItem.setTitle(R.string.action_turn_checker_on);
}
}

@Override
public boolean onOptionsItemSelected(MenuItem item) {
// Handle action bar item clicks here. The action bar will
// automatically handle clicks on the Home/Up button, so long
// as you specify a parent activity in AndroidManifest.xml.

switch(item.getItemId())
{
case R.id.action_settings:
return true;

case R.id.action_alarm_toggle:
boolean turnAlarmOn = !CommuteCheckAlarmService.isServiceAlarmOn(getActivity());
CommuteCheckAlarmService.setServiceAlarm(getActivity(), turnAlarmOn, savedUserInfo);

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB)
{
getActivity().invalidateOptionsMenu();
}

default:
return super.onOptionsItemSelected(item);
}
}

@Override
public void onActivityResult(int requestCode, int resultCode, Intent data)
{
if (resultCode != Activity.RESULT_OK)
{
return;
}
else
{
UserData tempUser = new UserData();
tempUser.copyUserData((UserData)data.getSerializableExtra(WorkCommuteDayAndTimeFragment.EXTRA_USER_DATA));

SimpleDateFormat sdf = new SimpleDateFormat("hh:mm aa ZZZZ", Locale.getDefault());
if (requestCode==REQUEST_WORK_COMMUTE)
    {
    if (tempUser.getDriveToWorkTime().toString() != null)
    {
         if (tempUser.getWorkWeek().size() > 0)
         {
         mUser.setWorkWeek(tempUser.getWorkWeek());
         }
    mUser.setDriveToWorkTime(tempUser.getDriveToWorkTime());
    mDisplayWorkCommute.setText((sdf.format(mUser.mDriveToWorkTime.getTime()) +", " + mUser.workWeekString()));
    }
}

if (requestCode==REQUEST_HOME_COMMUTE)
{
    if (tempUser.getDriveToHomeTime().toString() != null)
    {
    mUser.setDriveToHomeTime(tempUser.getDriveToHomeTime());
    mDisplayHomeCommute.setText(sdf.format(mUser.mDriveToHomeTime.getTime()));
    }
}
}
}

public boolean loadSavedInfo(String filename) throws StreamCorruptedException, IOException, ClassNotFoundException
{
savedUserInfo = new UserData();
ObjectInputStream file;

try
{
file = new ObjectInputStream((new FileInputStream(new File(new File(getActivity().getApplicationContext().getFilesDir(),"")+File.separator+USER_INFO_FILE))));
savedUserInfo.copyUserData((UserData)file.readObject());
file.close();
return true;
}
catch (FileNotFoundException e)
{
e.printStackTrace();
}
return false;
}

public void saveInfo(UserData user) throws IOException
{
ObjectOutput output = null;

if (savedUserInfo.equals(null))
{
savedUserInfo = new UserData();
}
savedUserInfo.copyUserData(user);

CommuteCheckAlarmService.setServiceAlarm(getActivity(), CommuteCheckAlarmService.isServiceAlarmOn(getActivity()), savedUserInfo);

try
{
output = new ObjectOutputStream( new FileOutputStream(new File(getActivity().getApplicationContext().getFilesDir(),"")+File.separator+USER_INFO_FILE));
output.writeObject(savedUserInfo);
output.close();
}
catch (FileNotFoundException e)
{
e.printStackTrace();
}
catch (IOException e)
{
e.printStackTrace();
}
}
}

This is the code for the IntentService that actually runs the alarm:

public class CommuteCheckAlarmService extends IntentService {

private static final String TAG = "MapQueryService";
public static final String PREF_IS_ALARM_ON="isServiceAlarmOn";
private static final String START_ADDRESS="work address";
private static final String END_ADDRESS="home address";

public CommuteCheckAlarmService(String name) {
super(TAG);
}
@Override
protected void onHandleIntent(Intent intent) {

String startAddress = intent.getStringExtra(START_ADDRESS);
String endAddress = intent.getStringExtra(END_ADDRESS);
Geocoder startGeocoder = new Geocoder(this, Locale.getDefault());
Geocoder endGeocoder = new Geocoder(this, Locale.getDefault());
double startLatitude=0.0;
double startLongitude=0.0;
double endLatitude=0.0;
double endLongitude=0.0;
try
{
List<Address> startGeocoderAddress = startGeocoder.getFromLocationName(startAddress, 1);
startLatitude = startGeocoderAddress.get(0).getLatitude();
startLongitude = startGeocoderAddress.get(0).getLongitude();
List<Address> endGeocoderAddress = endGeocoder.getFromLocationName(endAddress, 1);
endLatitude = endGeocoderAddress.get(0).getLatitude();
endLongitude = endGeocoderAddress.get(0).getLongitude();
}
catch (IOException e)
{
e.printStackTrace();
}

Uri startAndEndUri = Uri.parse("http://maps.google.com/maps?saddr="+startLatitude+","+startLongitude+"&daddr="+endLatitude+","+endLongitude);
Intent mapIntent = new Intent(android.content.Intent.ACTION_VIEW, startAndEndUri);
mapIntent.setPackage("com.google.android.apps.maps");
PendingIntent notificationIntent = PendingIntent.getActivity(this, 0, mapIntent, PendingIntent.FLAG_UPDATE_CURRENT);

Notification notification = new NotificationCompat.Builder(this)
.setTicker("Time to check your commute!")
.setSmallIcon(android.R.drawable.ic_dialog_alert)
.setContentTitle("CommuteWatcher")
.setContentText("It's time to check your commute! Tap this notification to start.")
.setContentIntent(notificationIntent)
.setAutoCancel(true)
.build();
NotificationManager notificationManager = (NotificationManager)getSystemService(NOTIFICATION_SERVICE);
notificationManager.notify(0, notification);
}

public CommuteCheckAlarmService()
{
super(TAG);
}

public static void setServiceAlarm(Context context, boolean isOn) throws StreamCorruptedException, IOException, ClassNotFoundException
{

//this overloaded method is since a BroadcastReceiver's lifecycle isn't really meant for file I/O, so it can just call this method, which will in turn
//call the same method the mainfragment would normally call.
UserData tempSavedUserInfo = new UserData();
ObjectInputStream file;

try
{
file = new ObjectInputStream((new FileInputStream(new File(new File(context.getFilesDir(),"")+File.separator+MainFragment.USER_INFO_FILE))));
tempSavedUserInfo.copyUserData((UserData)file.readObject());
file.close();
}
catch (FileNotFoundException e)
{
e.printStackTrace();
}

setServiceAlarm(context, isOn, tempSavedUserInfo);
}

public static void setServiceAlarm(Context context, boolean isOn, UserData userInfo)
{


Intent workCommuteIntent = new Intent(context, CommuteCheckAlarmService.class);
workCommuteIntent.putExtra(START_ADDRESS, userInfo.getHomeAddress());
workCommuteIntent.putExtra(END_ADDRESS, userInfo.getWorkAddress());
PendingIntent pendingWorkCommuteIntent = PendingIntent.getService(context, 0, workCommuteIntent, PendingIntent.FLAG_UPDATE_CURRENT);

Intent homeCommuteIntent = new Intent(context, CommuteCheckAlarmService.class);
homeCommuteIntent.putExtra(START_ADDRESS, userInfo.getWorkAddress());
homeCommuteIntent.putExtra(END_ADDRESS, userInfo.getHomeAddress());
PendingIntent pendingHomeCommuteIntent = PendingIntent.getService(context, 1, homeCommuteIntent, PendingIntent.FLAG_UPDATE_CURRENT);

AlarmManager alarmManager = (AlarmManager) context.getSystemService(Context.ALARM_SERVICE);

long workTime = userInfo.getDriveToWorkTime().getTimeInMillis();
long homeTime = userInfo.getDriveToHomeTime().getTimeInMillis();

if (isOn)
{
GregorianCalendar today = (GregorianCalendar) Calendar.getInstance();

for (Day day : userInfo.getWorkWeek())
if (day.get()==today.get(Calendar.DAY_OF_WEEK))
{
alarmManager.setRepeating(AlarmManager.RTC_WAKEUP, workTime, AlarmManager.INTERVAL_DAY, pendingWorkCommuteIntent);
alarmManager.setRepeating(AlarmManager.RTC_WAKEUP, homeTime, AlarmManager.INTERVAL_DAY, pendingHomeCommuteIntent);
}
}

else
{
alarmManager.cancel(pendingWorkCommuteIntent);
pendingWorkCommuteIntent.cancel();
alarmManager.cancel(pendingHomeCommuteIntent);
pendingHomeCommuteIntent.cancel();
}
PreferenceManager.getDefaultSharedPreferences(context)
.edit()
.putBoolean(PREF_IS_ALARM_ON, isOn)
.commit();

}

public static boolean isServiceAlarmOn(Context context)
{
Intent i = new Intent(context.getApplicationContext(), CommuteCheckAlarmService.class);
PendingIntent pi = PendingIntent.getService(context, 0, i, PendingIntent.FLAG_NO_CREATE);
return pi != null;
}
}
\$\endgroup\$
2
\$\begingroup\$

Indentation

I hope your code is properly indented in your IDE. Right now it looks unreadable. So unreadable, in fact, that I can't review your code properly. But let's see what we can find.

Unneeded parentheses

file = new ObjectInputStream((new FileInputStream(new File(new File(getActivity().getApplicationContext().getFilesDir(),"")+File.separator+USER_INFO_FILE))));

Huh, what's that (( doing there?

file = new ObjectInputStream(
 (
  new FileInputStream(
   new File(
    new File(
     getActivity().getApplicationContext().getFilesDir(),""
    )+File.separator+USER_INFO_FILE
   )
  )
 )
);

One of the layers is doing absolutely nothing! Remove it.

Bugs

if (savedUserInfo.equals(null))

Haaaang on.

Nice bug you've got there. If it's NOT null, it works as intended. But if savedUserInfo IS null, then you've got null.equals(null), which is not true - it's NullPointerException. And an app crash.

\$\endgroup\$
  • \$\begingroup\$ Indentation- Yeah, I kept screwing up the copying and pasting of code into here from github (link here if it's useful to you: github.com/professorpanic/CommuteWatcher), the formatting is much more readable in my IDE. parenthesis-I'm counting the parentheses and it looks like they're closing something, though I admit this is ugly to try and parse. I color-coded what is (hopefully) closing what: i57.tinypic.com/209t7wn.png. Bugs- Oh, crap. you're right. I'm a dolt, I'll fix that when I get home. Thank you! \$\endgroup\$ – Harry Tuttle Feb 11 '15 at 1:04
  • \$\begingroup\$ @HarryTuttle I thought it didn't need more explanation, but the red parentheses are useless. Get rid of them. \$\endgroup\$ – Pimgd Feb 11 '15 at 8:32
  • \$\begingroup\$ It was unclear since you said "them", but had "((" preceding. Either way, I cleaned it up. \$\endgroup\$ – Harry Tuttle Feb 11 '15 at 10:21
  • \$\begingroup\$ @HarryTuttle looking at your git repo, my indentation comment stands. Your IDE comes with an auto-formatter. Use it. \$\endgroup\$ – Pimgd Feb 11 '15 at 10:41
  • \$\begingroup\$ All right, cleaned up formatting. \$\endgroup\$ – Harry Tuttle Feb 11 '15 at 19:53
2
\$\begingroup\$

Performance: You should consider lazy loading by moving your findViewById calls to where you actually need them. The calls are very time consuming and delay displaying your Activity to the user.

Style: You should be consistent in declaring access modifiers. In your code you are sometimes declaring private and sometimes relying on the default access.

Also, by convention, constants should be public static, not private static. The point of private members is encapsulation, which is unnecessary, since the values never change.

And how come you are using Hungarian Notation for some of your variables and not others?

\$\endgroup\$
  • \$\begingroup\$ point 1. Sorry, I'm drawing a blank on how lazyloading these would be best. Where would it be most efficient for that to happen? I was under the impression that in onCreateView is where you need to make those assigning calls, when everything is being drawn to the user. point 2- you're right, that's something to be cleaned up. point 3- are you referring to the values that are mSomething? I just was operating under the impression that was something you assigned to simple values you'd want to get/set. I didn't set out for using Hungarian Notation. Never read up on it. Hooray, learning experience! \$\endgroup\$ – Harry Tuttle Feb 11 '15 at 0:51
  • \$\begingroup\$ Some people prefix members with m, local variables with l and parameters with p. Therefore it seems odd you are only doing so for some members and not for others. \$\endgroup\$ – barq Feb 11 '15 at 6:44
  • \$\begingroup\$ Hm. I only did it because the book and class I went through led me to believe that's how its always done. I'll clean up the preixes too. \$\endgroup\$ – Harry Tuttle Feb 11 '15 at 7:27

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.