edge555 / MyDay

Task Reminder App with Realtime Database (Firebase)
12 stars 6 forks source link

Some issues with MyDay app #1

Open atiqueahmedziad opened 4 years ago

atiqueahmedziad commented 4 years ago

Thank you for making this awesome app. I find your app very much useful in my day to day life. Well, I found a couple of issues upon checking the codebase. I am writing them below. Hoping that, these changes will make your app more awesome. πŸ˜„

Issue 1

If user chooses a date 02-04-2020, it is being saved in the database as 20200402. After reading this value from database, the incrementmonth() method return the date string as 2020502. The parsedate() method gets 2020502 as parameter and gives an exception saying StringIndexOutOfBoundsException since it is calculating the date here using wrong index values. All of these actually happens when Exampleadapter wants to set date in UI here. The app crashes due to this.

Issue 2

After adding a task, a user is able go back to Task Adder page pressing back button since you are not clearing the activity stack here If I go back using the back button, it shows me the existing data in the text fields that I just added while ago. If I save the task then, it actually adds a new task in database with same info which is redundant. Either it should update the same task OR user should not be able to go back to Task Adder Page by pressing back button after adding a task.

Issue 3

You may want to clear the activity stack and call finish() when user is detected in loginActivity. Also call finish() when a user signs in from loginActivity here Currently when I press the back button 2-3 times simultaneously, it takes me to login screen even though I am still signed in (since FirebaseAuth.getInstance().signOut() method is not being called upon pressing the back button).

Issue 4

I observed, after a certain time (most probably 1 min) the app screen flicks which probably due to refreshTask() method. You have used this method to check the tasks' time and send notification to users. It is reading the tasks' time from database in every one minute which is probably unnecessary and more likely dangerous.

It is unnecessary since the tasks are not likely to get updated in every minute. So, you can store & update the tasks in local database (SQLite) when addValueEventListener() fires and check the tasks' time periodically from local database. This will be more efficient way.

It is dangerous since you have a limited free quota in Firebase. You can only make 100 simultaneous connections in realtime database (even in cloud firestore database there is 50K/Day read limitations). Imagine your app having 1,000,000 users. Now guess how much will it cost you if each user make connection with the database and read from it every minutes! Remember you have addValueEventListener() method which is an asynchronous listener, listening to every data changes in your database. You may want to use it correctly in some places πŸ˜‰.

Suggestions

I think you had a tough time handling dates and times. In some places, I saw you making complex calculations to work with the dates and times. I think you can avoid all these complex calculation like here, even this. It is not a smart move to find the date, month, year, hour, min with substring method. Instead you could use SimpleDateFormat. I saw you had implemented SimpleDateFormat once in a method here but not sure why you didn't apply this to handle the dates and times all over your app.

The following approach will be more efficient in your use case with date and time. It will help you to avoid all the substring calculations for date, month, year, hour, minute.

// You will set DAY_OF_MONTH, MONTH, YEAR from DatePickerDialog
// You will set HOUR_OF_DAY, MINUTE from TimePickerDialog
Calendar calendar = Calendar.getInstance();
calendar.set(Calendar.DAY_OF_MONTH, datePicker.getDayOfMonth());
calendar.set(Calendar.MONTH, datePicker.getMonth());
calendar.set(Calendar.YEAR, datePicker.getYear());
calendar.set(Calendar.HOUR_OF_DAY, hour);
calendar.set(Calendar.MINUTE, minute);

// Save this value in your database. It's a long datatype.
// Read the `dateTimeInMillis` value from  database when you need date and time
long dateTimeInMillis = calendar.getTimeInMillis();

// Create a date object with dateTimeInMillis
Date date = new Date(dateTimeInMillis);

SimpleDateFormat formatter = new SimpleDateFormat("dd-MM-yyyy hh:mm:ss a z");

System.out.println("Date after formating: " + formatter.format(date));

Example: Let's say, user chooses date: 12 April 2020 with DatePickerDialog and time: 21:38 with TimePickerDialog. Then,

dateTimeInMillis = 1586705901015
Date after formating: 12-04-2020 09:38:21 PM GMT+06:00

So, you want to get the date, month and year by new SimpleDateFormat("dd-MM-yyyy");

You can parse each of these- Year with new SimpleDateFormat("yyyy"); Month number with new SimpleDateFormat("MM"); Month name (first 3 letter) with new SimpleDateFormat("MMM"); Day with new SimpleDateFormat("dd"); Hour with new SimpleDateFormat("hh"); Minute with new SimpleDateFormat("mm"); AM/PM with new SimpleDateFormat("a");

Another suggestion is to make Time Task and Reminder separate tabs instead of separate pages. I believe this will ensure a better UX (User Experience) in your app.

Nitpick

You may want to make the codes more readable. I get confused with some variable name like now and noww, also db and dbb.

If you have read so far, I want you to thank you for reading my comment patiently. I will be glad if my comment helps you to make your app better. If you have any questions, feel free to ask!

edge555 commented 4 years ago

Thank you so much for figuring this problems and explaining clearly. I was bit rushed when making this app also this is my first app. Maybe that's why missed few things. I will definitely follow your suggestions next time when I update it. Thank you again for taking your time to make this issue :)