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

refactor(notifications): Centralize notification state update logic #606

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cypherpepe
Copy link

The main improvement is the removal of repeated logic for filtering and updating notifications. This is achieved by centralizing it in the 'getUpdatedTopicState' function, making the reducer more readable and reducing potential errors from code duplication.

Description

This PR refactors the notifications reducer to improve code readability and reduce duplication. Previously, filtering and updating logic for notifications was duplicated in multiple cases (FETCH_NOTIFICATIONS_DONE, UNSHIFT_NEW_NOTIFICATIONS). This has now been centralized in a new helper function, getUpdatedTopicState, which handles filtering out existing notifications and updating the state consistently. As a result, the reducer is now cleaner, and the risk of introducing inconsistencies is minimized.

Type of change

Please put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (optimization of existing code)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Fixes/Resolves (Optional)

This PR does not fix any issues specifically, but it improves the maintainability of the code by reducing redundancy and simplifying state updates for notifications.

Examples/Screenshots (Optional)

Before After
Repeated logic in multiple cases Centralized logic in one function

Checklist:

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

The getUpdatedTopicState function improves consistency across state updates by ensuring that notifications are filtered and updated in a unified way. This reduces the risk of errors or bugs that could arise from duplicate logic in the reducer.

The main improvement is the removal of repeated logic for filtering and updating notifications. This is achieved by centralizing it in the 'getUpdatedTopicState' function, making the reducer more readable and reducing potential errors from code duplication.
Copy link

vercel bot commented Sep 24, 2024

@cypherpepe is attempting to deploy a commit to the Reown Team on Vercel.

A member of the Team first needs to authorize it.

@Vineetkg20
Copy link

"Great refactor! Centralizing the notification state update logic in getUpdatedTopicState really improves the readability and maintainability of the reducer. By reducing duplication, it lowers the chance of inconsistencies, which is fantastic. One suggestion: perhaps adding a few more comments or inline documentation explaining the function’s purpose could help new contributors understand the logic quicker. Have you also tested this thoroughly to ensure there are no edge cases that were missed? Overall, excellent improvement!"

Added comments to notificationsReducer for clarity
@cypherpepe
Copy link
Author

@Vineetkg20 Done

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