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

Moved UiStore #607

Closed
wants to merge 3 commits into from
Closed

Moved UiStore #607

wants to merge 3 commits into from

Conversation

yuriykulikov
Copy link
Owner

No description provided.

@@ -138,7 +141,6 @@ fun startKoin(context: Context): Koin {

return startKoin {
modules(module)
modules(AlarmsListActivity.uiStoreModule)
modules(loggerModule())
}
.koin

Choose a reason for hiding this comment

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

Based on the provided code patch, here's a brief code review:

  1. Imports:

    • The import for com.better.alarm.presenter.AlarmsListActivity has been removed.
  2. Changes in Koin module setup:

    • UiStore interface and its implementation UiStoreImpl have been added as singletons in the Koin module.
    • The module AlarmsListActivity.uiStoreModule has been removed.
  3. Other changes:

    • A UiStoreImpl instance is now created and injected when needed.
    • It seems that other parts of the code related to UiStore functionality have been added but are not shown in the provided patch.

Improvement suggestions:

  • Consider providing more context or details about the purpose and usage of the UiStore so it can be reviewed more effectively.
  • Ensure that the implementation of UiStoreImpl aligns with the intended functionality.

As for bug risks, without having complete visibility into the rest of the codebase and dependencies, it is difficult to identify specific bugs. However, ensure that all the necessary imports, dependencies, and modules are correctly defined and used throughout the codebase.

return UiStoreIR()
}
}

override fun onNewIntent(intent: Intent?) {
super.onNewIntent(intent)
logger.debug { "new $intent, ${intent?.extras}" }

Choose a reason for hiding this comment

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

Based on the code patch provided, here are some observations and suggestions for improvement:

  1. Remove unused imports: The code patch removes some unused imports (io.reactivex.subjects.PublishSubject, kotlinx.coroutines.flow.MutableStateFlow, org.koin.core.module.Module, org.koin.dsl.module). It's good to remove these to keep the code clean.

  2. Access modifiers: It's a good practice to use access modifiers explicitly. In this case, you have correctly made the uiStore property private by using the private modifier. You can do the same for other properties as well.

  3. Companion object: The code patch removes a companion object that defined a createStore function. Without additional context or understanding of how this function was used, it's difficult to provide specific guidance. Just ensure that the removal of the companion object doesn't introduce any issues elsewhere in the codebase.

Overall, the changes in the code patch seem focused on removing unused code and dependencies, which is generally a good practice. However, without a broader context, it's challenging to identify potential bug risks or other improvements. It's always helpful to consider factors like code readability, maintainability, and performance when reviewing code patches.

}

override var openDrawerOnCreate: Boolean = false
}

Choose a reason for hiding this comment

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

The code patch you provided appears to add functionality to the UiStore interface and implement it in the UiStoreImpl class. Here are some potential bug risks and improvement suggestions:

  1. It's generally a good practice to include import statements at the top of the file in alphabetical order for better organization. In this patch, the import statements are not sorted.

  2. It's always important to verify if the new imports (com.better.alarm.model.AlarmValue and com.better.alarm.model.Alarms) are correct and necessary for the implementation. Make sure they are used correctly within the code.

  3. For the editing property, consider using val instead of var if the value should not be modified after initialization in order to promote immutability and prevent accidental changes.

  4. When implementing the edit method, it would be a good idea to handle the case where alarms.getAlarm(id) returns null. You may want to add error handling or logging to handle such situations.

  5. In the hideDetails method, setting editing.value to null directly could potentially lead to multiple null assignments. Consider checking if editing.value is already null before making the assignment to prevent unnecessary updates.

  6. It's important to ensure that the id parameter passed to the edit method is valid. Consider adding input validation or error handling to handle the case where an invalid id is provided.

  7. Review the overall architecture and design to determine if the usage of PublishSubject and MutableStateFlow is appropriate and aligns with the intended behavior. Verify that they are used correctly and fulfill the requirements of the UI store.

  8. Make sure other parts of the codebase are updated accordingly to use the modified UiStore interface and the new implementation in UiStoreImpl.

These suggestions should help you improve the code and reduce potential bugs, but a more thorough review would require examining the entire codebase and understanding the context in which these changes are made.

@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Patch coverage: 90.47% and project coverage change: -0.06% ⚠️

Comparison is base (ea7ae96) 69.34% compared to head (b35aded) 69.28%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #607      +/-   ##
=============================================
- Coverage      69.34%   69.28%   -0.06%     
+ Complexity       582      581       -1     
=============================================
  Files             76       77       +1     
  Lines           4387     4389       +2     
  Branches         526      526              
=============================================
- Hits            3042     3041       -1     
  Misses          1015     1015              
- Partials         330      333       +3     
Files Changed Coverage Δ
...com/better/alarm/presenter/AlarmDetailsFragment.kt 68.57% <80.00%> (+0.52%) ⬆️
...rc/main/java/com/better/alarm/presenter/UiStore.kt 92.85% <92.85%> (ø)
...n/java/com/better/alarm/configuration/Container.kt 100.00% <100.00%> (ø)
...a/com/better/alarm/presenter/AlarmsListActivity.kt 72.95% <100.00%> (-1.91%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

showTimePicker()
viewModel.newAlarmPopupSeen = true
}

editedAlarmId = editedAlarm?.value?.id.takeIf { it != -1 }
Copy link

Choose a reason for hiding this comment

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

Based on the code patch you provided, here is a brief code review:

  1. Import Statements:

    • Ensure that all required import statements are present and correctly referenced.
    • In the given patch, the import statements for viewModels and ViewModel from the androidx.fragment.app package are added, indicating the usage of ViewModel in the code.
  2. AlarmDetailsViewModel:

    • A new class AlarmDetailsViewModel extending ViewModel is created.
    • It has a Boolean property newAlarmPopupSeen, which is initialized as false.
    • No methods or additional functionality are visible in the provided code, so it's difficult to evaluate the effectiveness of this class based on the patch alone.
  3. AlarmDetailsFragment:

    • The variable viewModel is added to the class using the viewModels() function from the androidx.fragment.app package, indicating the usage of the AlarmDetailsViewModel.
    • An instance of AlarmDetailsViewModel is created and assigned to the viewModel variable.
  4. onCreate() Method:

    • If the editedAlarm is new and viewModel.newAlarmPopupSeen is false, the showTimePicker() method is called and viewModel.newAlarmPopupSeen is set to true.
    • This logic suggests that the time picker will be displayed only once when a new alarm is edited for the first time.

Overall, it seems that the code patch introduces a new AlarmDetailsViewModel class and utilizes it in the AlarmDetailsFragment. The AlarmDetailsViewModel manages the state related to whether the new alarm popup has been seen or not. The code review does not reveal any specific bug risks or improvement suggestions as it only includes a limited portion of the codebase.

@@ -78,8 +78,8 @@
android {
compileSdk = 33
defaultConfig {
versionCode = 31502
versionName = "3.15.02"
versionCode = 31503

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.
@yuriykulikov yuriykulikov deleted the min-api-level-bump branch November 30, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant