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

Fixes #48, convert method to TryGet. #49

Closed
wants to merge 1 commit into from
Closed

Fixes #48, convert method to TryGet. #49

wants to merge 1 commit into from

Conversation

NN---
Copy link
Member

@NN--- NN--- commented Oct 24, 2018

No description provided.

protected internal virtual LambdaExpression TryGetConvertExpression(Type from, Type to)
/// <param name="expr">Expression that converts a value of type <i>TFrom</i> to <i>TTo</i> or null.</param>
/// <returns><see langword="true" /> for successful expression, <see langword="false" /> if failed..</returns>
protected internal bool TryGetConvertExpression(Type from, Type to, out LambdaExpression expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

CJ uses return null-as-none pattern in multiple places, not only in ConvertBuilder.
As for me there's no profit with converting these into Try(… out var).

Vote to decline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning null is not good idea and not suggested in most coding conventions.
Modern C# has out var which makes usage of TryGet functions very easy and safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest to call these methods with OrDefault suffix at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's continue here (rus).

Copy link
Member Author

Choose a reason for hiding this comment

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

@NN---
Copy link
Member Author

NN--- commented Nov 28, 2018

According to poll https://rsdn.org/poll/6564
Most people don't care and second/third places are with prefix or suffix naming.
This method already has Try prefix.

@NN--- NN--- closed this Nov 28, 2018
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.

None yet

2 participants