Обзор

Я работаю над Android-приложением To-Do. У меня есть фрагмент кода, который я использую для работы с задачами. Я новичок в разработке Android и хочу расти, поэтому мне бы очень хотелось, чтобы кто-нибудь оценил мой код и сказал, как я могу его улучшить.

Код

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")
        }
    }
}

Слово «менеджер» — это ловушка имени класса. Это расплывчато, и если у вас нет четкого представления о том, за что именно отвечает класс, вы рискуете нарушить принцип единой ответственности. У вас может возникнуть соблазн добавить слишком много функций в этот один класс.

Но в вашем случае вы используете его не так, как я обычно видел, когда вы храните список задач и выполняете с ним различные операции. Это все служебные функции для работы с произвольными List<Task>с. Они лучше подходят для функций расширения. Также нет причин иметь класс, если у него нет свойств. Затем вам нужно создать что-то без причины, просто чтобы использовать его функции.

Я бы назвал файл примерно так TaskLists.kt и сделайте эти функции расширения верхнего уровня с помощью приемника List<Task>. Что касается функций, которые работают с одной задачей, мне кажется, что почти все они должны быть просто функциями-членами класса Task.

isNeedShowNotification будет короче и легче / быстрее для понимания, если у вас есть служебная функция для определения того, что сейчас выходные. И это упрощает внесение изменений позже, например, если вы решите добавить локализацию, которая изменяет дни, считающиеся выходными.

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()
    }
}

В имени TaskTimeStatus можно было бы опустить слово Task, чтобы код было легче читать. В конце концов, само перечисление не имеет понятия о Задаче. Имея в названии слово Task, вы в конечном итоге засоряете свой код повсюду словом task как для класса Task, так и для перечисления TaskTimeStatus, что усложняет отслеживание.

Я думаю, что класс TasksForTimeStatus делает ваш код более сложным. Каждый раз, когда вы хотите распаковать список задач, связанных с каждым TaskTimeStatus, требуется дополнительный шаг. Название тоже многословное. Поскольку вы все равно передаете списки TasksForTimeStatus, я думаю, что было бы проще удалить этот класс и заменить ваше использование List<TasksForTimeStatus> с Map<TimeStatus, List<Task>>.

Затем, getTasksForTimeStatuses могло бы стать намного проще:

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
}

В getNewNotificationDate, вам следует использовать when блок как выражение вместо оператора, поскольку все ветки либо бросают, либо присваивают значение skipDays. В основном val skipDays = when //....

Для класса Task неверно переопределять toString() без отмены hashCode() чтобы соответствовать этому. Но в этом случае вы можете сделать это автоматически, пометив его как data учебный класс. Тогда copy функция тоже будет написана для вас.

И небольшой совет: функцию в компаньоне RepeatInterval можно упростить до:

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