Shush alarms UI by RohanBh · Pull Request #5 · samagra14/Shush

RohanBh · 2017-08-19T19:58:13Z

The work in this branch aims to complete basic functionality of shush alarms.

…Time List activity

RohanBh · 2017-09-01T19:48:11Z

@samagra14 please test and review this PR.

…eshed

samagra14

Please do the necessary changes and merge.

samagra14 · 2017-10-06T14:50:40Z

    </application>

-</manifest>
+</manifest>


Leave a line at EOF.

samagra14 · 2017-10-06T14:53:12Z

+        if (rowID == null || days.length != 7) {
+            return;
+        }
+        // Instance of calendar that holds current system time


Use a better variable name.

Will change it in next commit.

samagra14 · 2017-10-06T14:57:29Z

+     *                     scheduled for the corresponding day. i.e. days[0] is set to be true
+     *                     then a weekly alarm will be set such that it will shush the phone every Monday.
+     * @param rowID        Unique primary key of the shush alarm row.
+     */


Try using a different method name for both the methods to avoid confusion.

Reffering to this and this answer to the question When is method overloading appropriate?, the answers provided by various people suggest that overloading is appropriate when -

Each overloaded method should be functionally the same as the others in an overloaded group, otherwise it will be unclear as to how, when or why behaviour is being varied.

The second answer suggests that -

From the information given they appear to be equivalent (since one calls the other) and I would think it reasonable to overload them.

And since the methods I overloaded do exactly the same thing(since one calls the other), I think overloading can be used here.
What do you think @suyashmahar ?

I think it is right to use same name (this is what overloading is).

samagra14 · 2017-10-06T14:58:47Z

+     * @param endAlarmID        The id that uniquely identifies the end time alarm
     */
-    public void setAlarm(long startTimeInMillis, long endTimeInMillis) {
+    public void setAlarm(long startTimeInMillis, long endTimeInMillis, Integer startAlarmID, Integer endAlarmID) {


As stated above use different names for different methods.

suyashmahar

Why isn't an Alarm class used to pass data around in methods instead of passing them as individual parameters?

Nice job on formatting and comments! 🎉

suyashmahar · 2017-10-06T19:52:48Z

+     * @param rowID     Unique primary key of the shush alarm row.
+     */
+    public void setSingleDayAlarm(Calendar startTime, Calendar endTime, int day, Integer rowID) {
+        if (day > 6 || rowID == null) {


No exception?

If the method throws and exception, then I will have to catch it every time I use the method. I won't be passing anything that should cause an exception while using this method.

Isn't this a wrong way? You won't pass such values intentionally, but wouldn't it be better to catch that exception and show the user an error message or something?

This explains why there should be an exception. If by chance something happens and condition day > 6 or rowID == null is evaluated to true then the user wouldn't know that there is some problem. He wouldn't know if his action set an alarm or not.

suyashmahar · 2017-10-06T19:55:57Z

+     *                  then a weekly alarm will be set such that it will shush the phone every Monday.
+     * @param rowID     Unique primary key of the shush alarm row.
+     */
+    public void setWeeklyAlarm(Calendar startTime, Calendar endTime, boolean[] days, Integer rowID) {


Why isn't an exception thrown or logged here?

I will log in the error stream in next commit.

Throwing an exception would be better, logging in error stream decreases performance and won't be different from current code (functionality wise), programming defensively is much better.

suyashmahar · 2017-10-06T19:57:49Z

+            endTime.add(Calendar.DAY_OF_MONTH, 1);
+        }
+
+        // get Day index of start Time


Though nitpicking using i for variables that are used method wide isn't a good choice IMO

How can this cause a problem? Can you please elaborate?

Not a logical problem but it easier to move with the flow of code if variable names are descriptive.

suyashmahar · 2017-10-06T20:00:37Z

+                        endTime.getTimeInMillis(),
+                        getStartDayID(j, rowID),
+                        getEndDayID(j, rowID));
+            }


Is there any reason for two loops with similar function, why can't they be merged into a single loop?

Thanks for pointing this out.

RohanBh · 2017-10-07T14:42:34Z

@suyashmahar I can not find any Alarm class in android.

suyashmahar · 2017-10-07T14:55:17Z

+
+        mContext.getContentResolver().update(
+                Uri.withAppendedPath(PlacesContract.TimeEntry.CONTENT_URI, String.valueOf(holder.id))
+                , values, null, null


though works, line beginning with comma should be avoided

suyashmahar · 2017-10-07T14:59:34Z

+    private String getColumnFromDay(int day) {
+        switch (day) {
+            case 0: {
+                return PlacesContract.TimeEntry.COLUMN_MONDAY;


Why isn't an array of TimeEntry.<something> is used and then accessed by its index? That would have been an easier way for this.

To remember the names of the columns and to make the code readable this is used. For instance, COLUMN_WEDNESDAY seems more readable than column[2].

Are you sure? Currently, your code is spanning over 450 lines and everything is repeated 6 times. Using arrays and enums help you write code faster, read it faster and make it shorter that is what they are for.

suyashmahar · 2017-10-07T15:01:01Z

+        CheckBox thursday;
+        CheckBox friday;
+        CheckBox saturday;
+        CheckBox sunday;


An array of Checkboxes would be easier and smaller solution, this way you can iterate over each checkbox in a loop.

This is done to avoid arrays in the TimeEntry class so that the database contract is more readable.

why not use an array of strings then?

suyashmahar · 2017-10-07T15:03:16Z

+                context.getSystemService(Context.NOTIFICATION_SERVICE);
+
+        notificationManager.notify(0, builder.build());
+    }


https://stackoverflow.com/a/121967/6556360

@samagra14 ^

suyashmahar

Code for TimeListAdapter.java is seriously messed up.

suyashmahar · 2017-10-10T13:36:27Z

+     *                  then a weekly alarm will be set such that it will shush the phone every Monday.
+     * @param rowID     Unique primary key of the shush alarm row.
+     */
+    public void setWeeklyAlarm(Calendar startTime, Calendar endTime, boolean[] days, Integer rowID) {


Throwing an exception would be better, logging in error stream decreases performance and won't be different from current code (functionality wise), programming defensively is much better.

suyashmahar · 2017-10-10T13:41:24Z

+            endTime.add(Calendar.DAY_OF_MONTH, 1);
+        }
+
+        // get Day index of start Time


Not a logical problem but it easier to move with the flow of code if variable names are descriptive.

suyashmahar · 2017-10-10T13:42:29Z

+     *                     scheduled for the corresponding day. i.e. days[0] is set to be true
+     *                     then a weekly alarm will be set such that it will shush the phone every Monday.
+     * @param rowID        Unique primary key of the shush alarm row.
+     */


I think it is right to use same name (this is what overloading is).

suyashmahar · 2017-10-10T13:46:39Z

+                context.getString(R.string.alarm_intent_int_extra_key), -1);
+
+        if (timeInMillis == -1 || alarmID == -1){
+            return;


What if time or alarmID is -ve?

alarmID is SQLite generated primary key and is auto incremented. timeInMillis is a millisecond value that has passed since the epoch. Therefore, neither of them can be negative.

See this.

An instant in time can be represented by a millisecond value that is an offset from the Epoch, January 1, 1970 00:00:00.000 GMT (Gregorian).

suyashmahar · 2017-10-10T13:51:21Z

+
+            // Retrieving the whole row from the database.
+            int id = timeDataCursor.getInt(
+                    timeDataCursor.getColumnIndexOrThrow(PlacesContract.TimeEntry._ID)


Can this method call throw an exception?

In theory yes it can, but since we are using the column index from the contract class, we can be sure that the method call will not result in an unhandled exception state.

suyashmahar · 2017-10-10T14:02:59Z

+    private String getColumnFromDay(int day) {
+        switch (day) {
+            case 0: {
+                return PlacesContract.TimeEntry.COLUMN_MONDAY;


Are you sure? Currently, your code is spanning over 450 lines and everything is repeated 6 times. Using arrays and enums help you write code faster, read it faster and make it shorter that is what they are for.

suyashmahar · 2017-10-10T14:05:24Z

+                return PlacesContract.TimeEntry.COLUMN_SATURDAY;
+            }
+            case 6: {
+                return PlacesContract.TimeEntry.COLUMN_SUNDAY;


Things like this case statement would be just three lines, instead of current 20+ lines and would decrease writing and testing time. Testing because currently, you have 7 possible points of failure instead of 1.

I have used an array of checkboxes now, I will push the code soon. It did reduce many lines of code 😄

suyashmahar · 2017-10-10T14:06:20Z

+        CheckBox thursday;
+        CheckBox friday;
+        CheckBox saturday;
+        CheckBox sunday;


why not use an array of strings then?

RohanBh added 10 commits August 19, 2017 15:08

Modify build.gradle

fbf4c62

Complete Time list adapter and create time picker dialog fragment

9350e01

Add new methods to AlarmScheduler

a9c7f0b

Correct wrong column name of primary key in query and update methods

cfcfe65

Add time list activity and update adapter code

ff2d145

Resolve minor bug in AlarmScheduler

11741ad

Move send notification method to utils

788ba57

Refractor the code

dd97b69

Make time picker display the previous time in the textview

772eac5

Use exact alarms api to schedule shush alarms and add back button in …

82a3904

…Time List activity

RohanBh changed the title ~~[WIP] Shush alarms UI~~ Shush alarms UI Sep 1, 2017

RohanBh added 3 commits September 2, 2017 01:10

Use sound, vibrate and light with notification

413bd34

Close cursor in onStop instead of onDestroy

86aa4da

Only the Changed value is now updated instead of whole row being refr…

985c319

…eshed

samagra14 requested changes Oct 6, 2017

View reviewed changes

suyashmahar reviewed Oct 6, 2017

View reviewed changes

suyashmahar suggested changes Oct 7, 2017

View reviewed changes

Refractor the code

1e00d44

suyashmahar suggested changes Oct 10, 2017

View reviewed changes

RohanBh added 2 commits December 10, 2017 20:17

Use meaningful variable name for method wide used variables

502504f

Seperate the Adapter and Viewholder class to increase readability

24a8f00

Conversation

RohanBh commented Aug 19, 2017

Uh oh!

RohanBh commented Sep 1, 2017

Uh oh!

samagra14 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RohanBh Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suyashmahar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RohanBh commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suyashmahar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RohanBh Oct 7, 2017 •

edited

Loading

RohanBh Oct 10, 2017 •

edited

Loading

RohanBh Oct 10, 2017 •

edited

Loading