Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Fix to Map Value Objects #58

Merged
merged 4 commits into from
Aug 9, 2017
Merged

Conversation

nicolastakashi
Copy link
Contributor

Based on the code snippet that @g-adolph put on issue #46 discussion thread.

Now we can map properties of value objects.

Based on the code snippet that @g-adolph put on issue henkmollema#46 discussion thread.

Now we can map properties of value objects.
@henkmollema
Copy link
Owner

@nicolastakashi thanks for this! Have you verified that Dapper maps the columns to the desired value object?

@nicolastakashi
Copy link
Contributor Author

Yes @henkmollema
I wrote a test to verify the type mapped.
And everything looks like good

Copy link
Owner

@henkmollema henkmollema left a comment

Choose a reason for hiding this comment

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

Please take a look at the review comments.

var paramType = lambda.Parameters[0].Type;
var memberInfo = paramType.GetMember(baseMember.Name)[0];
return memberInfo;
paramType = lambda.Parameters.FirstOrDefault().Type;
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep using array indexer here:

paramType = lambda.Parameters[0].Type;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one question is there any reason not to use FirstOrDefault?

Is the idea really popping an exception if it does not have index 0?

Copy link
Owner

Choose a reason for hiding this comment

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

When reading the code FirstOrDefault tells you that there might not be a value present or that the collection is not an array/list. Beside that, I tend to avoid overhead from LINQ as much as possible.

Copy link
Owner

Choose a reason for hiding this comment

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

And this will throw an exception anyway, because you will be accessing Type from a null reference. I rather get an ArgumentOutOfRangeException than NullReferenceException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good!

Copy link
Owner

Choose a reason for hiding this comment

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

Can you FirstOrDefault() to [0] here?

Copy link
Owner

Choose a reason for hiding this comment

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

@nicolastakashi this one.

var memberInfo = paramType.GetMember(baseMember.Name)[0];
return memberInfo;
paramType = lambda.Parameters.FirstOrDefault().Type;
return paramType.GetMember(baseMember.Name).FirstOrDefault();
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep using array indexer here:

return paramType.GetMember(baseMember.Name)[0];

@@ -31,11 +33,22 @@ public static MemberInfo GetMemberInfo(LambdaExpression lambda)
case ExpressionType.MemberAccess:
var memberExpression = (MemberExpression)expr;
var baseMember = memberExpression.Member;
Type paramType;

while (!(memberExpression is null))
Copy link
Owner

Choose a reason for hiding this comment

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

while (memberExpression != null)

paramType = memberExpression.Type;
if (paramType.GetMembers().Any(member => member.Name == baseMember.Name))
{
return paramType.GetMember(baseMember.Name).FirstOrDefault();
Copy link
Owner

Choose a reason for hiding this comment

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

return paramType.GetMember(baseMember.Name)[0];

Copy link
Contributor Author

@nicolastakashi nicolastakashi left a comment

Choose a reason for hiding this comment

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

Perfect!

We can continue with these suggestions.

@henkmollema
Copy link
Owner

@nicolastakashi you might want to check your Git settings because you've been committing with different usernames/email addresses.

@nicolastakashi
Copy link
Contributor Author

I did not change my username or email.

I just checked and everything seems like before.

@henkmollema
Copy link
Owner

Take a look at the two commits here: https://github.com/henkmollema/Dapper-FluentMap/pull/58/commits. They show a different user with a different email address (and profile picture).

@nicolastakashi
Copy link
Contributor Author

Ow Really!

Something strange with Github, because the only thing I just did was actually change my profile picture, nothing more.

Do not know exactly where to fix it?

Any suggestion?

@nicolastakashi
Copy link
Contributor Author

If you search by my name you will see that only my profile appears.

@nicolastakashi
Copy link
Contributor Author

Would you like me to pull a pull request again?

Do you think this is an impediment?

@henkmollema
Copy link
Owner

No it's fine. Please fix the remaining comment and I'll merge this in.

@nicolastakashi
Copy link
Contributor Author

Which comment do you need to fix? Sorry, I do not understand very well.

@henkmollema
Copy link
Owner

There is one FirstOrDefault() left which should be [0] in stead.

Remove FirstOrDefault to keep code pattern
Copy link
Contributor Author

@nicolastakashi nicolastakashi left a comment

Choose a reason for hiding this comment

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

I Remove the FirstOrDefault funcition

var paramType = lambda.Parameters[0].Type;
var memberInfo = paramType.GetMember(baseMember.Name)[0];
return memberInfo;
paramType = lambda.Parameters.[0].Type;
Copy link
Owner

Choose a reason for hiding this comment

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

Parameters.[0] should be Parameters[0]

Copy link
Owner

@henkmollema henkmollema left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@henkmollema henkmollema merged commit d396ba7 into henkmollema:master Aug 9, 2017
@lepiroupo
Copy link

this version is already on nuget?

@henkmollema
Copy link
Owner

@lepiroupo @nicolastakashi I've just pushed v1.5.4 to NuGet: https://www.nuget.org/packages/Dapper.FluentMap/1.5.4.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants