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.

Sign up for GitHub

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

Jump to bottom

Extract and pull up assignFreeId from CAO/SAO #13577

Closed
wants to merge 1 commit into from
Closed

Extract and pull up assignFreeId from CAO/SAO #13577

wants to merge 1 commit into from

Conversation

JosiahWI
Copy link
Contributor

This unifies most of the duplicated logic in registerObject and should make further refactoring easier.

@JosiahWI JosiahWI marked this pull request as draft June 10, 2023 21:14
@JosiahWI
Copy link
Contributor Author

This PR goes along with #13560 and whichever one gets merged second will need a rebase.

@wsor4035 wsor4035 added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Jun 11, 2023
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Dec 26, 2023
@Zughy
Copy link
Member

Zughy commented Jan 15, 2024

@JosiahWI the aforementioned PR has been merged, are we ready to proceed here?

@JosiahWI
Copy link
Contributor Author

Yes we are.

@JosiahWI
Extract and pull up assignFreeId from CAO/SAO
0809d8e 
This unifies most of the duplicated logic in registerObject and should
make further refactoring easier.
@JosiahWI JosiahWI marked this pull request as ready for review January 15, 2024 19:00
@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label Jan 15, 2024
@Desour
Copy link
Member

Desour commented Jan 16, 2024

Sorry, but I think this makes the code harder to read and understand.
It moves code that is only relevant for registerObject into 4 different member function procedures, where some of them are even polymorphic. => Worse locality.
And if we ever wanted to change the code independently in client and server aomgr, we'd get into troubles.
The risk of accidentally changing one and not the other is low, because we know there are two so similar classes.
Overly detailed analysis ends here.

@JosiahWI JosiahWI closed this Jan 16, 2024
@JosiahWI
Copy link
Contributor Author

If someone could glance over PRs and make these comments before the author invests hours maintaining and rebasing the PR, that would be very helpful.

@Desour
Copy link
Member

Desour commented Jan 16, 2024

Yes, sorry! I lost track of this PR.
I'm currently taking a measure to get better in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Possible close
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JosiahWI @Zughy @Desour @SmallJoker @wsor4035