Skip to content

♻️ (frontend) Make the code more readable by simplifying conditional logic #844

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

r00gm
Copy link

@r00gm r00gm commented Apr 6, 2025

Use a map to make the code more readable

Purpose

Make the code more readable by simplifying conditional logic.

Proposal

Replaced nested ternary expressions with a mapping object to improve readability and maintainability.

  • ♻️ (frontend) Replaced conditional logic for title assignment with a key-based map lookup
  • ♻️ (frontend) Improved clarity and made it easier to add more filters in the future

Use a map to make the code more readable
@r00gm r00gm changed the title ♻️ Refactor DocxGrid.tsx ♻️ (frontend) Refactor DocxGrid.tsx Apr 6, 2025
@r00gm r00gm changed the title ♻️ (frontend) Refactor DocxGrid.tsx ♻️ (frontend) Make the code more readable by simplifying conditional logic Apr 6, 2025
@r00gm
Copy link
Author

r00gm commented Apr 6, 2025

Minor nitpick.
Couldn't stand the ternary nesting 😅 — replaced with a map before it summoned the ternary demon 😄

@lunika lunika requested a review from Copilot April 6, 2025 10:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment on lines +53 to 54
}[target]

Copy link
Preview

Copilot AI Apr 6, 2025

Choose a reason for hiding this comment

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

Directly indexing the mapping with [target] assumes that target always matches one of the map keys. Consider adding a default fallback (e.g., using the nullish coalescing operator) to prevent 'title' from being undefined for unexpected target values.

Suggested change
}[target]
}[target] ?? t('Unknown');

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Member

Choose a reason for hiding this comment

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

It was just to test how copilot does a review 🙂

Maybe based on the suggestion uses t('All docs') than t('Unknown'). WDYT @AntoLC ?

Copy link
Author

Choose a reason for hiding this comment

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

i would keep it the way it is.

if someone where to add another key to the enum DocDefaultFilter, typescript will raise an error which will force the addition of a new key to the translation map

Copy link
Author

Choose a reason for hiding this comment

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

or

Suggested change
}[target]
const title = {
[DocDefaultFilter.MY_DOCS]: t('My docs'),
[DocDefaultFilter.SHARED_WITH_ME]: t('Shared with me'),
}[target] ?? t('All docs');

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