Code for working with a tasks in To-Do Android application

Question

Overview

I am working on an android To-Do app. I have a piece of code that I use to work with tasks.I am new to android development and want to grow, so I would really like someone to rate my code and tell me how i can make it better.

Code

TaskManager.kt

class TaskManager {

    fun isNeedShowNotification(task: Task): Boolean {
        return when (task.repeatInterval) {
            RepeatInterval.Once -> true
            RepeatInterval.EveryDay -> true
            RepeatInterval.Weekends -> return LocalDate.now().dayOfWeek == DayOfWeek.SATURDAY || LocalDate.now().dayOfWeek == DayOfWeek.SUNDAY
            RepeatInterval.WorkingDays -> return LocalDate.now().dayOfWeek != DayOfWeek.SATURDAY && LocalDate.now().dayOfWeek != DayOfWeek.SUNDAY
        }
    }

    fun getInProgressTaskTasksByList(tasks: List<Task>, listId: Long): List<TasksForTimeStatus> {
        return getTasksForTimeStatuses(getInProgressTasksWithListId(tasks, listId))
    }

    fun getFinishedTasksForTimeStatuses(tasks: List<Task>): List<TasksForTimeStatus> {
        return getTasksForTimeStatuses(getFinishedTasks(tasks))
    }

    fun getInProgressTasksForTimeStatuses(tasks: List<Task>): List<TasksForTimeStatus> {
        return getTasksForTimeStatuses(getInProgressTasks(tasks))
    }

    private fun getTasksForTimeStatuses(tasks: List<Task>): List<TasksForTimeStatus> {
        val timeStatuses: MutableList<TasksForTimeStatus> = ArrayList()

        tasks.sortedBy { task -> task.notificationDate.epochMillis }
            .forEach {
                if (timeStatuses.size == 0) {
                    addNewTasksTimeStatus(timeStatuses, it)
                } else {
                    if (timeStatuses.last().taskTimeStatus == getTaskTimeStatus(it))
                        addTask(timeStatuses, it)
                    else {
                        addNewTasksTimeStatus(timeStatuses, it)
                    }
                }
            }

        return timeStatuses
    }

    private fun getInProgressTasksWithListId(tasks: List<Task>, listId: Long): List<Task> {
        return getInProgressTasks(tasks).filter { it.taskList == listId }
    }

    private fun getInProgressTasks(tasks: List<Task>): List<Task> {
        return getTasks(tasks).filter { task -> task.status == TaskStatus.InProgress }
    }

    private fun getFinishedTasks(tasks: List<Task>): List<Task> {
        return getTasks(tasks).filter { task -> task.status == TaskStatus.Finished }
    }

    private fun getTasks(tasks: List<Task>): List<Task> {
        return tasks.filter { task -> !task.isDeleted }
    }

    private fun getTaskTimeStatus(task: Task): TaskTimeStatus {
        val timeNow = Date(LocalDateTime.now())
        val timeTomorrow = timeNow + TimeUnit.DAYS.toMillis(1)

        return when {
            task.notificationDate.epochMillis - timeNow.epochMillis < 0 -> TaskTimeStatus.Overdue
            timeNow.toLongString() == task.notificationDate.toLongString() -> TaskTimeStatus.Today
            timeTomorrow.toLongString() == task.notificationDate.toLongString() -> TaskTimeStatus.Tomorrow
            task.notificationDate.epochMillis - TimeUnit.DAYS.toMillis(30) <= timeNow.epochMillis -> TaskTimeStatus.ThisMonth
            else -> TaskTimeStatus.Later
        }
    }

    private fun addNewTasksTimeStatus(timeStatuses: MutableList<TasksForTimeStatus>, task: Task) {
        timeStatuses.add(TasksForTimeStatus(getTaskTimeStatus(task)))
        addTask(timeStatuses, task)
    }

    private fun addTask(timeStatuses: MutableList<TasksForTimeStatus>, task: Task) {
        timeStatuses.last().addTask(task)
    }

    fun finishTaskByNotification(task: Task): Boolean {
        if (task.repeatInterval == RepeatInterval.Once)
            finishTask(task)
        else
            task.notificationDate = getNewNotificationDate(task)
        task.isFIncisedOnce = true

        return task.status == TaskStatus.Finished
    }

    private fun finishTask(task: Task) {
        if (task.status != TaskStatus.Finished)
            task.status = TaskStatus.Finished
        else
            task.status = TaskStatus.InProgress
    }

    private fun getNewNotificationDate(task: Task): Date {
        val skipDays: Long
        when (task.repeatInterval) {
            RepeatInterval.EveryDay -> skipDays = 1
            RepeatInterval.WorkingDays -> {
                skipDays = when (task.notificationDate.getAsLocalDate().dayOfWeek) {
                    DayOfWeek.SATURDAY -> 2
                    DayOfWeek.FRIDAY -> 3
                    else -> 1
                }
            }
            RepeatInterval.Weekends -> {
                skipDays = when (task.notificationDate.getAsLocalDate().dayOfWeek) {
                    DayOfWeek.THURSDAY -> 2
                    DayOfWeek.WEDNESDAY -> 3
                    DayOfWeek.TUESDAY -> 4
                    DayOfWeek.MONDAY -> 5
                    else -> 1
                }
            }
            else -> throw Exception("TaskManager do not know how get time for new notification for task with repeat interval - " + task.repeatInterval.name)
        }
        return task.notificationDate + TimeUnit.DAYS.toMillis(skipDays)
    }
}

Task.kt

 @Entity
    @Parcelize
    class Task(
        @PrimaryKey(autoGenerate = true) var id: Int = 0,
        var taskList: Long = 0,
        var taskText: String = "",
        var repeatInterval: RepeatInterval = RepeatInterval.Once,
        var notificationDate: Date = Date(0),
        var isNotificationNeed: Boolean = false,
        var status: TaskStatus = TaskStatus.InProgress,
        var taskColor: Int = -1972243,
        var isFIncisedOnce: Boolean = false,
        var isDeleted: Boolean = false
    ) : Parcelable {
        fun copy() = Task(id, taskList, taskText, repeatInterval, notificationDate, isNotificationNeed, status,taskColor,isFIncisedOnce,isDeleted)

        override fun equals(other: Any?): Boolean {
            if (other == null || other !is Task) return false
            return id == other.id && taskList == other.taskList && taskText == other.taskText && repeatInterval.id == other.repeatInterval.id &&
                    notificationDate.epochMillis == other.notificationDate.epochMillis && isNotificationNeed == other.isNotificationNeed &&
                    status.name == other.status.name && taskColor == other.taskColor && isDeleted == other.isDeleted
        }
 
    }

TasksForTimeStatus.kt

    class TasksForTimeStatus(var taskTimeStatus:TaskTimeStatus){
        val tasks: MutableList<Task> = ArrayList()
    
        fun addTask(task:Task){
            tasks.add(task)
        }
    }

TaskTimeStatus.kt

enum class TaskTimeStatus (private val stringId:Int){
    Overdue(R.string.overdue),
    Today(R.string.today),
    Tomorrow(R.string.tomorrow),
    ThisMonth(R.string.thismonth),
    Later(R.string.later);

    fun getString(context: Context): String {
        return context.getString(stringId)
    }

}

RepeatInterval.kt

enum class RepeatInterval(val id: Int,private val stringId:Int) {
    Once(0, R.string.repeat_interval_once),
    EveryDay(1,R.string.repeat_interval_every_day),
    WorkingDays(2,R.string.working_days_interval),
    Weekends(3,R.string.weekends_interval);

    fun toString(context: Context): String {
        return context.getString(stringId)
    }

    companion object {
        fun getById(id: Int): RepeatInterval {
            values().forEach {
                if (it.id == id)
                    return it
            }
            throw Exception("Cant find RepeatInterval with id - $id")
        }
    }
}

Answer 1

The word "manager" is a class name trap. It's vague and if you don't have a clear idea of exactly what the class is responsible for, you're in danger of violating the single responsibility principle. You might be tempted to add too much functionality into this one class.

But in your case, you're not using it the way I've typically seen, which would be storing the list of tasks and doing various operations on it. These are all utility functions for working with arbitrary List<Task>s. They are better suited to be extension functions. There's also no reason to have a class if it has no properties. Then you have to instantiate something for no reason just to use its functions.

I would name the file something like TaskLists.kt and make these top level extension functions with receiver List<Task>. For the functions that work with a single Task, pretty much all of them look to me like they should simply be member functions of the Task class.

isNeedShowNotification will be shorter and easier/faster to comprehend if you have a utility function for determining if it's the weekend. And it makes it easier to modify later, like if you decide to add a localization that changes which days are considered weekend.

private fun isNowWeekend(): Boolean = 
    LocalDate.now().dayOfWeek.let { it == DayOfWeek.SATURDAY || it == DayOfWeek.SUNDAY }

fun Task.isNeedShowNotification: Boolean {
    return when (task.repeatInterval) {
        RepeatInterval.Once, RepeatInterval.EveryDay -> true
        RepeatInterval.Weekends -> isNowWeekend()
        RepeatInterval.WorkingDays -> !isNowWeekend()
    }
}

The name TaskTimeStatus could drop the word Task to make code easier to read. After all, the enum itself has no notion of a Task. With the word Task in its name, you end up littering your code all over the place with the word task both for the Task class and the TaskTimeStatus enum, making it harder to follow.

I think the TasksForTimeStatus class contributes to your code being more complicated. It takes an extra step each time you want to unpack the list of Tasks that go with each TaskTimeStatus. The name is also wordy. Since you pass around lists of TasksForTimeStatus, anyway, I think it would be simpler to eliminate this class and replace your uses of List<TasksForTimeStatus> with Map<TimeStatus, List<Task>>.

Then, getTasksForTimeStatuses could become much simpler:

private fun List<Task>.byCurrentTimeStatus(): Map<TaskTimeStatus, List<Task>> {
    return tasks.sortedBy { task -> task.notificationDate.epochMillis }
        .groupBy(Task::getCurrentTimeStatus)
}

private fun Task.getCurrentTimeStatus(): TimeStatus {
    // your existing getTaskTimeStatus as an extension function with clearer name
}

In getNewNotificationDate, you should use the when block as an expression instead of a statement since all branches either throw or assign a value to skipDays. Basically val skipDays = when //....

For the Task class, it is incorrect to override toString() without overriding hashCode() to match it. But in this case, you can have it done for you automatically by marking it as a data class. Then the copy function will be written for you, too.

And a minor tip, the function in RepeatInterval's companion could be simplified to:

    fun getById(id: Int): RepeatInterval {
        return values().firstOrNull { it.id == id } 
            ?: error("Cant find RepeatInterval with id - $id")
    }