Skip to content

Refactor updateAlarmProfiles to Use currentProfile #720

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jyotiraditya-chauhan
Copy link

  • Changes
  • Refactored updateAlarmProfiles to use currentProfile for validation.
  • Added a check to ensure currentProfile exists before proceeding with updates.
  • Kept currentProfileName in the filter as it matches the AlarmModel.profile field type.
  • Improved code readability and maintainability by removing unused variable ambiguity.
  • Impact
  • Prevents potential silent failures if the current profile doesn’t exist.
  • Enhances code clarity for future contributors.
  • Verified that alarms update correctly when the profile exists.
  • Confirmed the function exits early with a message when currentProfile is null.

Thank you for reviewing this PR! Let me know if any adjustments are needed.

@jyotiraditya-chauhan jyotiraditya-chauhan changed the title This PR addresses an issue in the updateAlarmProfiles function where the currentProfile variable was declared but unused, creating ambiguity and redundancy. The function relied solely on currentProfileName without validating the profile's existence. Refactor updateAlarmProfiles to Use currentProfile Mar 3, 2025
@jyotiraditya-chauhan
Copy link
Author

Please review my PR

final currentProfile = await IsarDb.getProfile(currentProfileName);
if (currentProfile == null) {

print("Got Error: Current profile '$currentProfileName' not found.");
Copy link
Member

Choose a reason for hiding this comment

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

make it debugPrint. Ideally we should bring in logging into the application, but this will work for now.

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.

2 participants