WIP: Element Builder proposal #741

tdurieux · 2016-07-05T09:25:36Z

I created this PR in order to discuss the architecture of a possible new Element Builder in spoon.

The current factories are neither easy to use nor explicit. This new builder aims to solve these issues.
But I don't know yet the best solution.

This PR contains a POC of the builder. I would like to have your opinion.

Syntaxe

BinaryOperator

Builder B = factory.Builder();
CtBinaryOperator binary = B.Binary(
        B.Literal(2))
        .plus(B.Literal(1))
        .lower(
                B.Binary(B.Literal(10))
                        .minus(B.Literal(5))
        ).build();
Assert.assertEquals("(2 + 1) < (10 - 5)", binary.toString());

Try

Builder B = factory.Builder();
CtTry aTry = B.Try()
        .inBody(B.Increment(x.Literal(1)))
        .createCatch()
            .parameter("e", IllegalArgumentException.class)
            .inBody(x.Increment(x.Literal(1)))
            .close()
        .createCatch()
            .parameter("e", Exception.class)
            .close()
        .inFinally(x.Decrement(x.Literal(1)))
.build();
Assert.assertEquals("try {\n"
        + "    1++;\n"
        + "} catch (java.lang.IllegalArgumentException e) {\n"
        + "    1++;\n"
        + "} catch (java.lang.Exception e) {\n"
        + "} finally {\n"
        + "    1--;\n"
        + "}", aTry.toString());

Method

Builder x = factory.Builder();

        CtReturn<Object> aReturn = factory.Core().createReturn();
        aReturn.setReturnedExpression(x.Literal(1).build());
        CtMethod m = x.Method("m")
                .type(int.class)
                .setPrivate()
                .setStatic()
                .addParam(int.class, "x")
                .inBody(aReturn).build();
        Assert.assertEquals("private static int m(int x) {\n"
                + "    return 1;\n"
                + "}", m.toString());

Limitations

The builder requires a method to indicate the end of the element (here CatchBuilder.close()) in order to get back the parent Builder (here TryBuilder). It is not intuitive...
This architecture will introduce a lot of duplicate codes. Example: all elements that have a body will have the method inBody. Unfortunately the inheritance will not solve this issue (in this case we need multi inheritance)

alcides · 2016-07-05T09:29:12Z

Love it! I really need this!

On Tue, Jul 5, 2016 at 10:25 AM, Thomas Durieux [email protected]
wrote:

I created this PR in order to discuss the architecture of a possible new
Element Builder in spoon.

The current factories are neither easy to use nor explicit. This new
builder aims to solve these issues.
But I don't know yet the best solution.

This PR contains a POC of the builder. I would like to have your opinion.
Syntaxe BinaryOperator

CtBinaryOperator binary = B.Binary(
B.Literal(2))
.plus(B.Literal(1))
.lower(
B.Binary(B.Literal(10))
.minus(B.Literal(5))
).build();Assert.assertEquals("(2 + 1) < (10 - 5)", binary.toString());

Try

CtTry aTry = x.Try()
.inBody(x.Increment(x.Literal(1)))
.createCatch()
.parameter("e", IllegalArgumentException.class)
.inBody(x.Increment(x.Literal(1)))
.close()
.createCatch()
.parameter("e", Exception.class)
.close()
.inFinally(x.Decrement(x.Literal(1)))
.build();Assert.assertEquals("try {\n"
+ " 1++;\n"
+ "} catch (java.lang.IllegalArgumentException e) {\n"
+ " 1++;\n"
+ "} catch (java.lang.Exception e) {\n"
+ "} finally {\n"
+ " 1--;\n"
+ "}", aTry.toString());

Limitations

The builder requires a method to indicate the end of the element (here
CatchBuilder.close()) in order to get back the parent Builder (here
TryBuilder). It is not intuitive...
2.

This architecture will introduce a lot of duplicate codes. Example:
all elements that have a body will have the method inBody.
Unfortunately the inheritance will not solve this issue (in this case we
need multi inheritance)

You can view, comment on, or merge this pull request online at:

#741
Commit Summary

feat(builder): WIP Builder proposal

File Changes

A src/main/java/spoon/reflect/builder/AbsBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-0 (71)

A src/main/java/spoon/reflect/builder/BinaryOperatorBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-1 (156)

A src/main/java/spoon/reflect/builder/Builder.java
https://github.com/INRIA/spoon/pull/741/files#diff-2 (104)

A src/main/java/spoon/reflect/builder/CatchBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-3 (71)

A src/main/java/spoon/reflect/builder/IfBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-4 (89)

A src/main/java/spoon/reflect/builder/LiteralBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-5 (27)

A src/main/java/spoon/reflect/builder/MethodBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-6 (103)

A src/main/java/spoon/reflect/builder/TryBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-7 (84)

A src/main/java/spoon/reflect/builder/UnaryOperatorBuilder.java
https://github.com/INRIA/spoon/pull/741/files#diff-8 (37)

M src/main/java/spoon/reflect/factory/Factory.java
https://github.com/INRIA/spoon/pull/741/files#diff-9 (3)

M src/main/java/spoon/reflect/factory/FactoryImpl.java
https://github.com/INRIA/spoon/pull/741/files#diff-10 (12)

A src/test/java/spoon/reflect/builder/BinaryBuilderTest.java
https://github.com/INRIA/spoon/pull/741/files#diff-11 (45)

A src/test/java/spoon/reflect/builder/TryBuilderTest.java
https://github.com/INRIA/spoon/pull/741/files#diff-12 (38)

Patch Links:

https://github.com/INRIA/spoon/pull/741.patch

https://github.com/INRIA/spoon/pull/741.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#741, or mute the thread
https://github.com/notifications/unsubscribe/AAAbsSKQFnnsZWCVFtpQVTYkMPvDiI2Fks5qSiMQgaJpZM4JE7ge
.

monperrus · 2016-07-05T09:45:57Z

looks like javapoet for Spoon?

tdurieux · 2016-07-05T09:47:31Z

May be, I did not check.

GerardPaligot · 2016-07-05T09:48:37Z

@monperrus Yes but better, with love and for Spoon! <3

monperrus · 2016-07-05T11:41:11Z

src/test/java/spoon/reflect/builder/BinaryBuilderTest.java

+	public void simpleBinaryTest() {
+		Factory factory = new Launcher().getFactory();
+		Builder B = factory.Builder();
+		CtBinaryOperator binary = B.Binary(


infix notation? Lit(2).times(Lit(3))

monperrus · 2016-07-05T11:50:24Z

I love this idea. It will be an excellent alternative for intercession in addition to the core intercession API, snippets and templates.

monperrus · 2017-01-04T20:46:04Z

generate the builder with https://github.com/google/FreeBuilder?

pvojtechovsky · 2017-02-11T08:03:02Z

The builder requires a method to indicate the end of the element (here CatchBuilder.close()) in order to get back the parent Builder (here TryBuilder). It is not intuitive...

What about API like this?

Builder B = factory.Builder();
CtTry aTry = B.Try()
        .inBody(B.Increment(x.Literal(1)))
        .createCatch(B.Catch("e", IllegalArgumentException.class)
            .inBody(x.Increment(x.Literal(1)))
        ).createCatch(B.Catch("e", Exception.class)
        ).inFinally(x.Decrement(x.Literal(1)))
.build();

the close() can be avoided by building whole catch content as a parameter of createCatch(...)

tdurieux · 2017-02-11T08:08:54Z

Why not but there are a lot of parenthesis and call to the factory (that I was trying to avoid)

pvojtechovsky · 2017-02-11T08:15:01Z

I do not prefer any yet. Just giving ideas into this nice concept, to help to move it forward 👍

CtTry aTry = B.Try();
aTry.inBody(B.Increment(x.Literal(1)))
aTry.createCatch()
            .parameter("e", IllegalArgumentException.class)
            .inBody(x.Increment(x.Literal(1)))
aTry.createCatch()
            .parameter("e", Exception.class)
aTry.inFinally(x.Decrement(x.Literal(1)))
aTry.build();

or just rename of methods?

CtTry aTry = B.Try()
        .inBody(B.Increment(x.Literal(1)))
        .begingCatch()
            .parameter("e", IllegalArgumentException.class)
            .inBody(x.Increment(x.Literal(1)))
        .end()
        .beginCatch()
            .parameter("e", Exception.class)
        .end()
        .inFinally(x.Decrement(x.Literal(1)))
.build();

tdurieux · 2017-02-11T08:27:30Z

I do not prefer any yet. Just giving ideas into this nice concept, to help to move it forward

Yes, it is very hard to create a good design for this builder.

or just rename of methods?

I prefer this solution

pvojtechovsky · 2017-02-11T09:36:51Z

there are a lot of parenthesis and call to the factory (that I was trying to avoid)

May be we can avoid other calls of factory too

CtTry aTry = B.Try()
        .beginTry()
            .Increment(x.Literal(1)))
        .endTry()
        .beginCatch()...

the endTry(), cannot be replaced by end(), because endTry() returns CtTryBuilder instance. The consistency of begin/end pairs would be assured in runtime by builder.

But I agree that this idea is really wild. So parenthesis and use of factory is better in this case, because we are building another spoon model element.

So the rule would be:

building of each spoon model element starts on factory
fluent API is used to fill all attributes of that one spoon model element

tdurieux · 2017-02-11T09:44:54Z

I would said:

CtTry aTry = B.Try()
        .beginBody()
            .Increment(x.Literal(1)))
        .endBody()
        .beginCatch()...

pvojtechovsky · 2017-02-11T09:50:30Z

This architecture will introduce a lot of duplicate codes.

it is no problem, if code of builder will be generated.

pvojtechovsky · 2017-02-11T09:58:59Z

CtTry aTry = B.Try()
.beginBody()
   .Increment(x.Literal(1)))
.endBody()
.beginCatch()
    .parameter("e", IllegalArgumentException.class)
    .beginBody()
       .Increment(x.Literal(1)))
     .endBody()
.endCatch()

The problem of this API is "What is the return type of endBody() method"? Will be the same endBody method used in building of CtMethod?
A) If endBody returns generic SpoonModelBuilder class which is model element independent, then it there will be a lot of methods and API client will be not guided.
B) If endBody returns CtTry specific CtTryBuilder class, then how will be the Increment(..) method implemented to be able to return CtTryBuilder ??
Therefore I suggested endTry ... but I do not think that it is good way at all.

tdurieux · 2017-02-11T10:55:11Z

You are right, I did not think enough

pvojtechovsky · 2017-06-28T20:41:03Z

One more idea ... to use lambda expression to generate nested elements. Like this:

CtTry aTry = factory.Builder().Try()
        .inBody(body ->
             body.Increment(x.Literal(1)))
        .createCatch(IllegalArgumentException.class, e, body->
             body.statement(st -> st.Increment(x.Literal(1)))
                     .statement(st -> st.invoke("java.lang.System.out.println()"))
        ).createCatch(Exception.class, "e")
        .inFinally(body -> 
             body.statemenet(st -> st.Decrement(x.Literal(1)))
.build();

autocompletion will work on each level
builder code has no duplicities

tdurieux · 2017-06-29T09:01:24Z

I can be a solution but it is only nice for Java 8.
In Java 7 it is not practicable but I don't know if it is a problem

tdurieux force-pushed the wip-feat-builder branch from 3cd8af0 to 07c192e Compare July 5, 2016 09:26

tdurieux force-pushed the wip-feat-builder branch from 07c192e to 35a5fbb Compare July 5, 2016 09:30

tdurieux force-pushed the wip-feat-builder branch from 35a5fbb to 8776689 Compare July 5, 2016 11:27

monperrus reviewed Jul 5, 2016
View reviewed changes

monperrus changed the title ~~Element Builder proposal~~ WIP: Element Builder proposal Jul 5, 2016

feat(builder): WIP Builder proposal

7411209

tdurieux force-pushed the wip-feat-builder branch from 8776689 to 7411209 Compare July 5, 2016 12:15

monperrus closed this Aug 16, 2016

pvojtechovsky mentioned this pull request Feb 11, 2017

feature: Spoon metamodel #1183

Closed

2 tasks

pvojtechovsky mentioned this pull request Oct 2, 2018

WIP: Refactor return type of metamodel methods #1852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Element Builder proposal #741

WIP: Element Builder proposal #741

tdurieux commented Jul 5, 2016 •

edited

Loading

alcides commented Jul 5, 2016

monperrus commented Jul 5, 2016

tdurieux commented Jul 5, 2016 •

edited

Loading

GerardPaligot commented Jul 5, 2016

monperrus Jul 5, 2016

monperrus commented Jul 5, 2016

monperrus commented Jan 4, 2017

pvojtechovsky commented Feb 11, 2017

tdurieux commented Feb 11, 2017

pvojtechovsky commented Feb 11, 2017 •

edited

Loading

tdurieux commented Feb 11, 2017 •

edited

Loading

pvojtechovsky commented Feb 11, 2017

tdurieux commented Feb 11, 2017 •

edited

Loading

pvojtechovsky commented Feb 11, 2017

pvojtechovsky commented Feb 11, 2017 •

edited

Loading

tdurieux commented Feb 11, 2017

pvojtechovsky commented Jun 28, 2017

tdurieux commented Jun 29, 2017

WIP: Element Builder proposal #741

WIP: Element Builder proposal #741

Conversation

tdurieux commented Jul 5, 2016 • edited Loading

Syntaxe

BinaryOperator

Try

Method

Limitations

alcides commented Jul 5, 2016

monperrus commented Jul 5, 2016

tdurieux commented Jul 5, 2016 • edited Loading

GerardPaligot commented Jul 5, 2016

monperrus Jul 5, 2016

Choose a reason for hiding this comment

monperrus commented Jul 5, 2016

monperrus commented Jan 4, 2017

pvojtechovsky commented Feb 11, 2017

tdurieux commented Feb 11, 2017

pvojtechovsky commented Feb 11, 2017 • edited Loading

tdurieux commented Feb 11, 2017 • edited Loading

pvojtechovsky commented Feb 11, 2017

tdurieux commented Feb 11, 2017 • edited Loading

pvojtechovsky commented Feb 11, 2017

pvojtechovsky commented Feb 11, 2017 • edited Loading

tdurieux commented Feb 11, 2017

pvojtechovsky commented Jun 28, 2017

tdurieux commented Jun 29, 2017

tdurieux commented Jul 5, 2016 •

edited

Loading

tdurieux commented Jul 5, 2016 •

edited

Loading

pvojtechovsky commented Feb 11, 2017 •

edited

Loading

tdurieux commented Feb 11, 2017 •

edited

Loading

tdurieux commented Feb 11, 2017 •

edited

Loading

pvojtechovsky commented Feb 11, 2017 •

edited

Loading