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

Move to Shared - SqlSer.cs #1313

Merged
merged 14 commits into from
Jan 12, 2022

Conversation

Kaur-Parminder
Copy link
Contributor

This is part of Code merge for issue #1261. I have created .common.cs and moved the uncommon code to private methods.

This is part of Code merge for issue dotnet#1261. I have created .common.cs and moved the uncommon code to private methods.
@Kaur-Parminder Kaur-Parminder added the ➕ Code Health Changes related to source code improvements label Oct 4, 2021
@DavoudEshtehari DavoudEshtehari added this to the 4.0.0-preview3 milestone Oct 4, 2021
@johnnypham
Copy link
Contributor

There's not that much code remaining in the framework specific files. Why don't we just move it to the common file?

Removed Netfx and Netcore files for SqlSer and added Netfx if endifs in common file
Just updating class not use partial anymore, indentation, comment correction
@Kaur-Parminder
Copy link
Contributor Author

There's not that much code remaining in the framework specific files. Why don't we just move it to the common file?

@johnnypham I just removed the netfx and netcore classes and updated SqlSer.cs in common project with the three functions.

  1. Both Serialize and DeSerialize functions uses If defs, added return statements in if block.
    2 .GetUdtAttribute function in netfx used Reflection to throw same exception so just kept the .NetCore one.

@cheenamalhotra cheenamalhotra removed this from the 4.0.0-preview3 milestone Oct 19, 2021
@JRahnama JRahnama added this to the 5.0.0-preview1 milestone Dec 7, 2021
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

The SerializationHelperSql9 could be sealed: internal sealed class SerializationHelperSql9
Also, I see some minor possible improvements like unnecessary assignment and redundant member. You can find them in the error list window.

Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

I'd suggest using Dictionary instead of Hashtable to avoid boxing and unboxing for s_types2Serializers:

  • private static ConcurrentDictionary<Type, Serializer> s_types2Serializers;

addressing review comments
Added check for key in dictionary and addressed review comments
@DavoudEshtehari DavoudEshtehari merged commit 9a4e332 into dotnet:main Jan 12, 2022
@Kaur-Parminder Kaur-Parminder deleted the Move-to-shared-SqlSer branch January 12, 2022 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants