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

Use XmlSerializer over BinaryFormatter #153

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

Emik03
Copy link
Member

@Emik03 Emik03 commented Mar 20, 2024

BinaryFormatter will become fully deprecated in .NET 9, and already throws exceptions in .NET 8 since it's opt-in now. This pull request makes migrations to .NET 8 and up possible.

XmlSerializer in this case is an easy swap-in replacement. There may be some performance implications, but I haven't noticed any regressions when previewing or playing charts in the Quaver game, including extremely heavy/dense charts.

@Warp9000
Copy link
Member

I feel like theres better ways of cloning objects than serializing and deserializing

@Emik03
Copy link
Member Author

Emik03 commented Mar 20, 2024

I feel like theres better ways of cloning objects than serializing and deserializing

Agreed! I would personally make every class derive from ICloneable and give deep clone implementations, however this is a refactor job that I would want explicit approval of since it's a larger time commitment than swapping 2 lines of code.

@Cuckson
Copy link

Cuckson commented Mar 20, 2024

I think XElement would be a bit more efficient than serialization if you're worried about the performance.

@Illuminati-CRAZ
Copy link
Member

The documentation for ICloneable recommends against implementing it for public APIs

@Illuminati-CRAZ Illuminati-CRAZ merged commit 0df5405 into Quaver:master Mar 25, 2024
1 check passed
@Emik03
Copy link
Member Author

Emik03 commented Mar 26, 2024

The documentation for ICloneable recommends against implementing it for public APIs

We can always make a ICopyable<T> or similar interface, or just straight up include .Clone()/.Copy() functions without any polymorphism capabilities. The point here more is that these classes should have dedicated functions to perform deep copies.

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.

4 participants