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

WithConstructor seems to resolve constructor parameters and only once #42

Closed
worldspawn opened this Issue Mar 11, 2016 · 14 comments

Comments

Projects
None yet
5 participants
@worldspawn
Copy link

worldspawn commented Mar 11, 2016

Consider this usage:

.CreateListOfSize(5)
.All()
.WithConstructor(() => new Registration(Faker.Internet.Email(), RegistrationType.Beneficiary))

Passes the same email to the constructor for every time it creates the object. It should not do that.

Your code

var constructorArguments =
                (from argument in ((NewExpression)constructor.Body).Arguments
                select Expression.Lambda(argument).Compile().DynamicInvoke()).ToArray();

Resolves once and uses the same args over and over. Seems quite inappropriate for what this library does and the efforts you've made elsewhere to introduce randomness. You discard the constructor expression but why not use it to instantiate the object?

Consider this class

public class User 
{
  public Foo(string emailAddress)
  {
    EmailAddress = emailAddress;
  }

  public string EmailAddress { get; protected set; }
}

So anywhere we have a class with a constructor that initialises a member that is protected it becomes unusable with the CreateListOfSize method. I'd need to loop myself and provide a different expression each time.

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Mar 11, 2016

This is an interesting idea, but I imagine the reason the WithConstructor logic was put into place was to handle situations in which the constructor depended on some external service:

.CreateListOfSize(5)
.All()
.WithConstructor(() =>  new Registration(new RegistrarService()) )
;

NBuilder wouldn't be able to introduce randomness into these kinds of constructors.
Thoughts?

@worldspawn

This comment has been minimized.

Copy link
Author

worldspawn commented Mar 11, 2016

I'm not sure that makes any difference. If it was the intention of the user to share the registrar service between instances then they can instantiate it outside of the expression and refer to the instance in the expression.

Randomness, at least in my example is introduced by me rather than nbuilder. Perhaps I am not understanding your point

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Mar 11, 2016

I think it was me who wasn't understanding your point. You're just saying you want the constructor expression to be recompiled and executed for each item in the list. That makes sense to me, though my gut says it should be optional.

@worldspawn

This comment has been minimized.

Copy link
Author

worldspawn commented Mar 11, 2016

I don't think it even needs to be recompiled. Compiled once to Func<T> and called for each, it should do what I want it to.

Other than perhaps being a breaking change in behavior I don't see why it should be optional. The caller will have the control over whether a value is reused of not.

New service per instance

WithConstructor(() =>  new Registration(new RegistrarService()) )

Shared instance

var svc = new RegistrarService();
//...
WithConstructor(() =>  new Registration(svc) )

But it's your library :)

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Mar 11, 2016

The breaking change is what I'm worried about. I see your point about just re-using the func. This is a good idea. Thanks!

@crmckenzie crmckenzie added this to the Future Version milestone May 1, 2016

@cullimorer

This comment has been minimized.

Copy link

cullimorer commented Aug 16, 2017

So was this ever implemented? I want the same, to construct an object with different values on each new instantiation. Is that possible?

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Aug 16, 2017

As implemented I can't really address this issue without making a breaking change to the Fluent API. Specifically, NBuilder is expecting a constructor expression as an argument to WithConstructor(). There are 2 issues with this:

  1. The expression is not actually a Func<T> which is what I was expecting when I read it.
  2. Func<T> cannot be easily converted to an Expression<Func>, which you can see by trying the following code:
            // does not compile
            var users = new Builder().CreateListOfSize<User>(2)
                .All()
                .WithConstructor(() =>
                    {
                        return new User($"Email{i}@nowhere.com")
                    })
                .Build()
                ;

The simpler solution would be to add a new method to the API called WithFactoryMethod() that just takes a Func<T>. This raises a question about what to do if the caller provides both WithConstructor() and WithFactoryMethod() in their build instructions. Which one takes precedent? Perhaps the solution is to throw an exception if the caller attempts to use both. I need to resolve this question before I can proceed.

Thoughts?

@PureKrome

This comment has been minimized.

Copy link
Contributor

PureKrome commented Aug 16, 2017

Bummer re: issue 2 because having a Func<T> argument with WithConstructor() would be soo much easier to read/grok vs another extension method.

(just thinking out a loud)

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Aug 16, 2017

yeah--but look how complex this part of the code already is:

        public T Construct(int index)
        {
            bool requiresArgs = reflectionUtil.RequiresConstructorArgs(typeof(T));

            if (typeof(T).IsInterface())
                throw new TypeCreationException("Cannot build an interface");

            if (typeof(T).IsAbstract())
                throw new TypeCreationException("Cannot build an abstract class");

            T obj;

            if (_constructorExpression != null)
            {
                obj = _constructorExpression.Compile().Invoke(index);
            }
            else if (requiresArgs && constructorArgs != null)
            {
                obj = reflectionUtil.CreateInstanceOf<T>(constructorArgs);
            }
            else if (constructorArgs != null)
            {
                obj = reflectionUtil.CreateInstanceOf<T>(constructorArgs);
            }
            else
            {
                obj = reflectionUtil.CreateInstanceOf<T>();
            }

            return obj;
        }

How many reflective if cases do you want to support? I'd much rather mark this method obsolete and point to the new (cleaner) method.

@PureKrome

This comment has been minimized.

Copy link
Contributor

PureKrome commented Aug 16, 2017

Can you make this method obsolete and then add a new one, though ALSO called WithConstructor(F<T>) ?

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Aug 16, 2017

I could--but it would violate the principle of least surprise. Further, there's nothing about Func<T> that implies or requires "constructor"--but Factory Method fits the bill exactly.

@cullimorer

This comment has been minimized.

Copy link

cullimorer commented Aug 17, 2017

Hey guys, apologies I'm not sure if I was confused by the thread topic. I was finding when I used the WithConstructor() and setting the value that it was using that value for all constructed entities, which isn't great for testing purposes. The way I got around it was to use Faker.NET with NBuilder but mock out the entity I wanted with a call to Faker to set the default property each time.

With the below, it was Person that didn't have a parameterless constructor but I got around it like so:

        public class MockPerson : Person
        {
            private MockPerson() : base(GetRandomName())
            {
            }

            private static PersonName GetRandomName()
            {
                return new PersonName(Faker.Name.First(), Faker.Name.Last(), AutomationNodeName.Unspecified);
            }
        }

Thank you for fast response though, good luck.

@fromm1990

This comment has been minimized.

Copy link

fromm1990 commented Aug 24, 2017

I also found the behavior, pointed out by the original comment, strange. When would it be convenient to have a list of size x of identical items?

However I came up with a temporary fix for my project:

First create an extension method

public static class ExtensionMethods
{
    public static IList<T> CreateListOfSize<T>(this Builder builder, int size, Expression<Func<T>> constructorBuilder)
    {
        var list = new List<T>();

        for (int i = 0; i < size; i++)
        {
            list.Add(builder.CreateNew<T>().WithConstructor(constructorBuilder).Build());
        }

        return list;
    }
}

Then initialize the Builder:

var builder = new Builder();
var list = builder.CreateListOfSize(10, () => new Coordinate2D(Faker.RandomNumber.Next(-90, 91), Faker.RandomNumber.Next(-180, 181)));

It should be mentioned that this strategy does not support .All, .With, .And etc. as the one shown in @cullimorer's comment would do, however, it will not require to mock every class that has a parameterized constructor.

crmckenzie added a commit that referenced this issue Jul 7, 2018

#42: address an [issue](#42) in which the constructor expression was …
…not being reevaluated for each item in a list.

@crmckenzie crmckenzie self-assigned this Jul 7, 2018

@crmckenzie crmckenzie modified the milestones: Future Version, 6.0.0 Jul 7, 2018

@crmckenzie

This comment has been minimized.

Copy link
Collaborator

crmckenzie commented Jul 7, 2018

I came to my senses. From the release notes:

  • Breaking Change: WithConstructor
    • No longer takes an Expression<Func<T>>.
    • Takes a Func<T>.
    • Marked [Obsolete] in favor of WithFactory
    • This change was to address an issue in which the constructor expression was not being reevaluated for each item in a list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.