Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite logic of habit status computation #4

Closed
wants to merge 3 commits into from
Closed

Conversation

DanielRendox
Copy link
Owner

@DanielRendox DanielRendox commented Dec 20, 2023

Type

Enhancement


Description

  • Refactor routine to habit
  • Use HabitStatus instead of HistoricalStatus and PlanningStatus
  • Add additional statuses for clarity
  • Rewrite logic of habit status computation so that both historical and future statuses can be computed on the go instead of being cached in the database. The only data that needs to be stored is the number of times completed.

PR changes walkthrough

Relevant files                                                                                                                                 
Enhancement
6 files
RoutineLocalDataSourceImpl.kt                                                             
    core/database/src/main/java/com/rendox/routinetracker/core/database/routine/RoutineLocalDataSourceImpl.kt

    Refactor routine to habit, Use HabitStatus instead of
    HistoricalStatus and PlanningStatus, Add additional statuses
    for clarity, Rewrite logic of habit status computation
+43/-43
ToggleHistoricalStatusUseCase.kt                                                       
    core/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/use_cases/ToggleHistoricalStatusUseCase.kt

    Update ToggleHistoricalStatusUseCase.kt
+32/-32
HabitComputeStatusUseCase.kt                                                               
    core/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/HabitComputeStatusUseCase.kt

    Add HabitComputeStatusUseCase.kt
+279/-0
Schedule.kt                                                                                                 
    core/model/src/main/java/com/rendox/routinetracker/core/model/Schedule.kt

    Update Schedule.kt
+36/-36
InsertRoutineStatusUseCase.kt                                                             
    core/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/use_cases/InsertRoutineStatusUseCase.kt

    Update InsertRoutineStatusUseCase.kt
+32/-32
GetRoutineStatusUseCase.kt                                                                   
    core/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/use_cases/GetRoutineStatusUseCase.kt

    Update GetRoutineStatusUseCase.kt
+22/-22
Tests
1 files
ScheduleIsDueTest.kt                                                                               
    core/domain/src/test/java/com/rendox/routinetracker/core/domain/routine/schedule/ScheduleIsDueTest.kt

    Update ScheduleIsDueTest.kt
+25/-25

refactor routine to habit;
use HabitStatus instead of HistoricalStatus and PlanningStatus;
add additional statuses for clarity;
rewrite logic of habit status computation so that both: historical and future statuses can be computed on the go instead of being cashed in the database, the only data that needs to be stored is number of times completed;
Repository owner deleted a comment from CodiumAI-Agent Dec 20, 2023
@DanielRendox
Copy link
Owner Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Refactoring and improving the logic of habit status computation
  • 📝 PR summary: This PR refactors the 'Routine' class to 'Habit' and rewrites the logic of habit status computation. It replaces 'HistoricalStatus' and 'PlanningStatus' with 'HabitStatus' and adds additional statuses for clarity. The computation of historical and future statuses is now done on the go instead of being cached in the database.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR contains a significant amount of changes in the logic of habit status computation and renaming of classes and variables, which requires a thorough understanding of the existing codebase to review effectively.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR includes a significant amount of refactoring and changes in the logic of habit status computation. It would be beneficial to include unit tests to ensure the new logic works as expected and to prevent potential regressions in the future. Additionally, it would be helpful to provide more context in the PR description about why these changes were necessary and how they improve the codebase.

  • 🤖 Code feedback:
    relevant filecore/database/src/main/java/com/rendox/routinetracker/core/database/routine/RoutineLocalDataSourceImpl.kt
    suggestion      Consider using a data mapping layer or a mapper function to convert between the database entity and the domain model. This can help to keep the conversion logic in one place and make the code more maintainable. [important]
    relevant lineimport com.rendox.routinetracker.core.model.Habit

    relevant filecore/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/use_cases/GetRoutineStatusUseCase.kt
    suggestion      It would be better to separate the logic of prepopulating history with missing dates into a separate function or use case. This would make the code more modular and easier to test. [important]
    relevant lineprivate suspend fun prepopulateHistoryWithMissingDates(habit: Habit, today: LocalDate) {

    relevant filecore/domain/src/test/java/com/rendox/routinetracker/core/domain/routine/schedule/ScheduleIsDueTest.kt
    suggestion      Consider using parameterized tests or data-driven tests for testing the same logic with different inputs. This can help to reduce the amount of repetitive test code and make the tests more readable. [medium]
    relevant lineimport com.rendox.routinetracker.core.domain.completion_history.schedule.isDue

    relevant filecore/domain/src/main/java/com/rendox/routinetracker/core/domain/completion_history/use_cases/GetRoutineStatusUseCase.kt
    suggestion      Consider using a strategy pattern or a similar design pattern for computing future statuses. This can help to make the code more flexible and easier to extend in the future. [medium]
    relevant lineprivate suspend fun computeFutureStatuses(

How to use

Instructions

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

Repository owner deleted a comment from CodiumAI-Agent Dec 20, 2023
Repository owner deleted a comment from CodiumAI-Agent Dec 20, 2023
@DanielRendox
Copy link
Owner Author

@CodiumAI-Agent /ask Is it reasonable to split this PR into smaller parts? And what parts exactly?

@CodiumAI-Agent
Copy link

Question: Is it reasonable to split this PR into smaller parts? And what parts exactly?

Answer:
Given the scope of the changes, it would be reasonable to split this PR into smaller parts. Here are some suggestions:

  1. Refactoring Routine to Habit: This includes all changes where the term 'Routine' is replaced with 'Habit'. This would involve changes in variable names, class names, and method names.

  2. Changes in Schedule properties: This includes changes where 'routineStartDate' and 'routineEndDate' are replaced with 'startDate' and 'endDate', and 'cancelDuenessIfDoneAhead' is replaced with 'completingAheadEnabled'.

  3. Changes in ScheduleIsDueTest: This part includes changes in the test cases related to the Schedule class.

  4. Changes in ToggleHistoricalStatusUseCase: This part includes changes in the use case related to toggling the historical status.

By splitting the PR in this way, each PR would have a clear, focused purpose, making it easier to review and understand.

Repository owner deleted a comment from CodiumAI-Agent Dec 20, 2023
@DanielRendox
Copy link
Owner Author

/describe

Copy link
Contributor

PR Description updated to latest commit (61698b6)

@github-actions github-actions bot added the enhancement New feature or request label Dec 20, 2023
@DanielRendox
Copy link
Owner Author

update_changelog

Copy link
Contributor

Changelog updates:

2023-12-20

Changed

  • Rewrite logic of habit status computation
  • Refactor routine to habit
  • Use HabitStatus instead of HistoricalStatus and PlanningStatus
  • Add additional statuses for clarity
  • Rewrite logic of habit status computation so that both historical and future statuses can be computed on the go instead of being cached in the database. The only data that needs to be stored is the number of times completed.

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

@DanielRendox
Copy link
Owner Author

/update_changelog --pr_update_changelog.push_changelog_changes=true

Comment on lines +1 to +8
## 2023-12-20

### Changed
- Rewrite logic of habit status computation
- Refactor routine to habit
- Use HabitStatus instead of HistoricalStatus and PlanningStatus
- Add additional statuses for clarity
- Rewrite logic of habit status computation so that both historical and future statuses can be computed on the go instead of being cached in the database. The only data that needs to be stored is the number of times completed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGELOG.md update

Repository owner deleted a comment from github-actions bot Dec 20, 2023
@DanielRendox
Copy link
Owner Author

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions

💡 Suggestion:

Rename the variable 'routineStartDate' to 'habitStartDate' to match the new naming convention.

File: core/domain/src/test/java/com/rendox/routinetracker/core/domain/routine/schedule/ScheduleIsDueTest.kt (28-28)

Example code:

Existing code:

val schedule: Schedule = Schedule.EveryDaySchedule(
    startDate = routineStartDate,
)

Improved code:

val schedule: Schedule = Schedule.EveryDaySchedule(
    startDate = habitStartDate,
)

💡 Suggestion:

Rename the variable 'routineId' to 'habitId' to match the new naming convention.

File: core/domain/src/main/java/com/rendox/routinetracker/core/domain/streak/StartStreakOrJoinStreaksUseCase.kt (21-23)

Example code:

Existing code:

val previousStreak = streakRepository.getStreakByDate(
    routineId = habit.id!!,
    dateWithinStreak = date.minus(DatePeriod(days = 1)),
)

Improved code:

val previousStreak = streakRepository.getStreakByDate(
    habitId = habit.id!!,
    dateWithinStreak = date.minus(DatePeriod(days = 1)),
)

💡 Suggestion:

Rename the variable 'routineId' to 'habitId' to match the new naming convention.

File: core/domain/src/main/java/com/rendox/routinetracker/core/domain/streak/StartStreakOrJoinStreaksUseCase.kt (64-66)

Example code:

Existing code:

val lastNotCompleted = completionHistoryRepository.getLastHistoryEntryByStatus(
    routineId = habit.id!!,
    matchingStatuses = listOf(HistoricalStatus.NotCompleted),
)

Improved code:

val lastNotCompleted = completionHistoryRepository.getLastHistoryEntryByStatus(
    habitId = habit.id!!,
    matchingStatuses = listOf(HistoricalStatus.NotCompleted),
)

💡 Suggestion:

Rename the variable 'routineId' to 'habitId' to match the new naming convention.

File: core/domain/src/main/java/com/rendox/routinetracker/core/domain/streak/StartStreakOrJoinStreaksUseCase.kt (77-82)

Example code:

Existing code:

streakRepository.insertStreak(
    routineId = habit.id!!,
    streak = Streak(
        startDate = streakStart,
        endDate = firstFailedStatusAfterCurrentDate
    )
)

Improved code:

streakRepository.insertStreak(
    habitId = habit.id!!,
    streak = Streak(
        startDate = streakStart,
        endDate = firstFailedStatusAfterCurrentDate
    )
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants