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

Add fluent combinator API #46

Closed
jdmarble opened this issue Feb 14, 2019 · 8 comments
Closed

Add fluent combinator API #46

jdmarble opened this issue Feb 14, 2019 · 8 comments

Comments

@jdmarble
Copy link

Testing Problem

I have some classes with many fields that are created using the builder pattern. When the number of fields is less than 9, I can create arbitraries easily enough:

Combinators.combine(
            Arbitraries.of(MyEnum.class),
            Arbitraries.integers().between(0, 100000),
            Arbitraries.strings().alpha().numeric(),
           //...
            ).as((a, b, c /*...*/) ->
                new Foo.Builder().
                    setA(a).
                    setB(b).
                    setC(c).
                    //...
                    build());

There's some extra boilerplate, but it's not a big problem.

When there are many fields, I've been using this pattern:

var builder = Arbitraries.of(new Foo.Builder());
builder = Combinators.combine(builder, Arbitraries.of(MyEnum.class)).
    as(Foo.Builder::setA);
builder = Combinators.combine(builder, Arbitraries.integers().between(0, 100000)).
    as(Foo.Builder::setB);
builder = Combinators.combine(builder, Arbitraries.strings().alpha().numeric()).
    as(Foo.Builder::setC);
//...
return builder.map(Foo.Builder::build);

I've further reduced boilerplate with this wrapper class:

public class FluentCombinator<T> implements Arbitrary<T> {
    private final Arbitrary<T> delegate;

    public FluentCombinator(Arbitrary<T> delegate) {
        this.delegate = delegate;
    }

    public <U, A1> FluentCombinator<U> map(Combinators.F2<T, A1, U> mapper, Arbitrary<A1> a1) {
        return new FluentCombinator<>(Combinators.combine(delegate, a1).as(mapper));
    }

    @Override
    public RandomGenerator<T> generator(int genSize) {
        return delegate.generator(genSize);
    }
}

This allows for some nicer code:

new FluentCombinator<>(Arbitraries.of(new Foo.Builder())).
            map(Foo.Builder::setA, Arbitraries.of(MyEnum.class)).
            map(Foo.Builder::setB, Arbitraries.integers().between(0, 100000)).
            map(Foo.Builder::setC, Arbitraries.strings().alpha().numeric()).
            /*...*/
            map(Foo.Builder::build);

Suggested Solution

I'd rather this be integrated into Arbitrary itself. Then I could just do:

Arbitraries.of(new Foo.Builder()).
            map(Foo.Builder::setA, Arbitraries.of(MyEnum.class)).
            map(Foo.Builder::setB, Arbitraries.integers().between(0, 100000)).
            map(Foo.Builder::setC, Arbitraries.strings().alpha().numeric()).
            /*...*/
            map(Foo.Builder::build);

Discussion

The current boilerplate per setter isn't that painful until you have many dozens of fields.

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2019

I like the idea. Just playing with potential APIs.

Consider those domain classes

public static class Person {
	private final String name;
	private final int age;
	public Person(String name, int age) {
		this.name = name;
		this.age = age;
	}
	@Override
	public String toString() {
		return String.format("%s (%s)", name, age);
	}
}

public static class PersonBuilder {
	private String name = "A name";
	private int age = 42;
	public PersonBuilder withName(String name) {
		this.name = name;
		return this;
	}
	public PersonBuilder withAge(int age) {
		this.age = age;
		return this;
	}
	public Person build() {
		return new Person(name, age);
	}
}

The test could look like this:

@Property
void fluentCombinators(@ForAll("people") Person aPerson) {
	System.out.println(aPerson);
}

@Provide
Arbitrary<Person> people() {
	BuilderCombinator<PersonBuilder> builderCombinator = Combinators.withBuilder(new PersonBuilder());
	Arbitrary<String> names = Arbitraries.strings().alpha().ofLength(10);
	Arbitrary<Integer> ages = Arbitraries.integers().between(0, 130);
	return builderCombinator
			   .use(names).in(PersonBuilder::withName)
			   .use(ages).in(PersonBuilder::withAge)
			   .build(PersonBuilder::build);
}

What do you think?

There's a potential problem with the builder instance being reused but one can solve that with lazy generation.

@jdmarble
Copy link
Author

That would work well for my use case. I'm slightly turned off by the separated use/in methods and the implications that has for the implementation, but I could live with it. :)

I was feeling pretty proud of myself with how general my original proposal was. Hypothetically, it could be useful for things other than working with builders. But, it's hard to synthesize another good use case for chaining combinators together.

@jlink
Copy link
Collaborator

jlink commented Feb 20, 2019

@jdmarble Could you elaborate the negative implications of use/in?

I introduced use/in because it reads a bit more fluent (for my taste) and it's a bit closer to 'Combinators.combine(...).as(...)'. And the generality that was in your approach is still preserved.

@jdmarble
Copy link
Author

jdmarble commented Feb 22, 2019

Yes, I can elaborate. Consider the return type of BuilderCombinator.use:

  • If it is BuilderCombinator then the argument to use must be stored as state for future calls. In my opinion, superfluous state is at least a small negative. How do we enforce that in is called after use and only one in is called before a use and the argument to use has a type that matches the argument of use etc... It seems there is no type safe (compile time checked) way to implement these constraints.
  • If it is some other type, say IntermediateBuilderCombinator, then it can be made to be type safe, but that is one extra class to implement, maintain, and understand. I'll admit that this is a small cost, but it should be weighed against the benefit of fluency.

Now I see that it is nearly as general as my approach. One thing that tripped me up was that the name (BuilderCombinator) makes it seem that it is only applicable to the builder pattern. This is, currently, my only use case, so I'm okay with that. In fact, I think that is a massive advantage for user understanding if it is the only thing it should be used for.

What if we want to combine more than two arbitraries? It would be difficult to do this in a type safe way without having IntermediateBuilderCombinator1, IntermediateBuilderCombinator2, etc... With an "extended map", it would look like:

    public <U, A1> Arbitrary<U> map(Combinators.F2<T, A1, U> mapper, Arbitrary<A1> a1) {
        return Combinators.combine(this, a1).as(mapper);
    }

    public <U, A1, A2> Arbitrary<U> map(Combinators.F3<T, A1, A2, U> mapper, Arbitrary<A1> a1, Arbitrary<A2> a2) {
        return Combinators.combine(this, a1, a2).as(mapper);
    }

    // ... and so on ...

Here's another (contrived) use case for this idea that isn't exactly a builder:

        return Combinators.withBuilder(Calendar.getInstance())
            
            .use(Arbitraries.integers().between(1990, 2000))
            .in((calendar, year) -> calendar.set(Calendar.YEAR, year))
            
            .use(Arbitraries.integers().between(5, 15))
            .in((calendar, weekOfYear) -> calendar.set(Calendar.WEEK_OF_YEAR, weekOfYear))
            
            .use(Arbitraries.integers().between(2, 5))
            .in((calendar, dayOfWeek) -> calendar.set(Calendar.DAY_OF_WEEK, dayOfWeek))
            
            .build(calendar -> calendar);

@jlink
Copy link
Collaborator

jlink commented Feb 23, 2019

@jdmarble Thanks for the valuable feedback.

A statically typed split between use and in requires an additional class indeed. This is a bit more complex to implement. The reason I prefer this API is the symmetry to Combinators.combine(..).as(..) which works the same way. I've come to prefer this style of fluent API in recent years since reasonable line breaking is a bit more obvious than breaking lines inbetween a parameter list.

With a single parameter the implementation cost is not very high either. Here's my prototypical implementation (I chose a new entrance class BuildCombinator):

public class BuilderCombinator<B> {
	private Arbitrary<B> builder;
	public static <B> BuilderCombinator<B> with(Supplier<B> builderSupplier) {
		return new BuilderCombinator<>(builderSupplier);
	}
	private BuilderCombinator(Supplier<B> builder) {
		this(Arbitraries.create(builder));
	}
	private BuilderCombinator(Arbitrary<B> delegate) {
		this.builder = delegate;
	}
	public <T> CombinableBuilder<B, T> use(Arbitrary<T> arbitrary) {
		return new CombinableBuilder<>(builder, arbitrary);
	}
	public <T> Arbitrary<T> build(Function<B, T> buildFunction) {
		return builder.map(buildFunction);
	}

	public static class CombinableBuilder<B, T> {
		private final Arbitrary<B> builder;
		private final Arbitrary<T> arbitrary;
		private CombinableBuilder(Arbitrary<B> builder, Arbitrary<T> arbitrary) {
			this.builder = builder;
			this.arbitrary = arbitrary;
		}
		public <C> BuilderCombinator<C> in(Combinators.F2<B, T, C> toFunction) {
			Arbitrary<C> arbitraryOfC = 
					builder.flatMap(b -> arbitrary.map(t -> toFunction.apply(b, t)));
			return new BuilderCombinator<>(arbitraryOfC);
		}
	}
}

Having a use with 2,3,.. parameters would require an additional CombinableBuilder per number of params.

As for naming, I'm a bit torn between BuilderCombinator, ArbitraryBuilder and something with "Fluent" in it.

My suggestion: I include something along the lines sketched above as experimental feature, and you - and others - can give me feedback about how it feels in real usage.

1 similar comment
@jlink
Copy link
Collaborator

jlink commented Feb 23, 2019

@jdmarble Thanks for the valuable feedback.

A statically typed split between use and in requires an additional class indeed. This is a bit more complex to implement. The reason I prefer this API is the symmetry to Combinators.combine(..).as(..) which works the same way. I've come to prefer this style of fluent API in recent years since reasonable line breaking is a bit more obvious than breaking lines inbetween a parameter list.

With a single parameter the implementation cost is not very high either. Here's my prototypical implementation (I chose a new entrance class BuildCombinator):

public class BuilderCombinator<B> {
	private Arbitrary<B> builder;
	public static <B> BuilderCombinator<B> with(Supplier<B> builderSupplier) {
		return new BuilderCombinator<>(builderSupplier);
	}
	private BuilderCombinator(Supplier<B> builder) {
		this(Arbitraries.create(builder));
	}
	private BuilderCombinator(Arbitrary<B> delegate) {
		this.builder = delegate;
	}
	public <T> CombinableBuilder<B, T> use(Arbitrary<T> arbitrary) {
		return new CombinableBuilder<>(builder, arbitrary);
	}
	public <T> Arbitrary<T> build(Function<B, T> buildFunction) {
		return builder.map(buildFunction);
	}

	public static class CombinableBuilder<B, T> {
		private final Arbitrary<B> builder;
		private final Arbitrary<T> arbitrary;
		private CombinableBuilder(Arbitrary<B> builder, Arbitrary<T> arbitrary) {
			this.builder = builder;
			this.arbitrary = arbitrary;
		}
		public <C> BuilderCombinator<C> in(Combinators.F2<B, T, C> toFunction) {
			Arbitrary<C> arbitraryOfC = 
					builder.flatMap(b -> arbitrary.map(t -> toFunction.apply(b, t)));
			return new BuilderCombinator<>(arbitraryOfC);
		}
	}
}

Having a use with 2,3,.. parameters would require an additional CombinableBuilder per number of params.

As for naming, I'm a bit torn between BuilderCombinator, ArbitraryBuilder and something with "Fluent" in it.

My suggestion: I include something along the lines sketched above as experimental feature, and you - and others - can give me feedback about how it feels in real usage.

@jlink
Copy link
Collaborator

jlink commented Mar 1, 2019

For the moment, I'll go with Combinators.withBuilder(...).use(...).in(...).build(...).

Implemented in a5348f2

Available in latest 1.1.1-SNAPSHOT

@jlink
Copy link
Collaborator

jlink commented Mar 15, 2019

Will be available in 1.1.1 (deployment ongoing). Feel free to reopen if something should be improved.

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

No branches or pull requests

2 participants