Skip to content

chore: use early return to improve readability of getAxisMapByAxes#3107

Merged
ckifer merged 1 commit into
recharts:masterfrom
neefrehman:chore/getAxisMapByAxes-early-return
Dec 23, 2022
Merged

chore: use early return to improve readability of getAxisMapByAxes#3107
ckifer merged 1 commit into
recharts:masterfrom
neefrehman:chore/getAxisMapByAxes-early-return

Conversation

@neefrehman

@neefrehman neefrehman commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

Follow-on from #3103

I've made a change to getAxisMapByAxes in generateCategoricalChart.tsx, to do an early return in the axes.reduce call when we have cached data in the accumulator, instead of at the end. This reduces the nesting of code to improve readability. It also avoids the function having to call getDisplayedData when not necessary—which also iterates over a list—to give a potential performance improvement if there are ever duplicated axisIdKeys.

@ckifer
ckifer requested a review from arcthur December 20, 2022 16:53

@ckifer ckifer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes look good to me. Just shifting inverting the return logic which is safe here.

We need to add unit tests to generateCategoricalChart but I don't think this PR should be responsible for that. It does get tested via other tests (since every categorical chart calls it and every one with an XAxis calls this specifically).

@ckifer ckifer added P1 High priority issues enhancement Enhancement to a current API refactor PRs that refactor existing code and removed enhancement Enhancement to a current API labels Dec 21, 2022
@ckifer

ckifer commented Dec 23, 2022

Copy link
Copy Markdown
Member

I pulled this down locally just to do some testing. This early return isn't ever reached in any of the recharts demos - inversion looks safe to merge.

@ckifer
ckifer merged commit b1b9963 into recharts:master Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High priority issues refactor PRs that refactor existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants