Comments on builder pattern?

[Eric Ayers]

Recently I've been wrapping some of my JavaScriptObjects using the builder pattern where an instance of the object you are setting is in the return value. 

  http://galgwt-reviews.appspot.com/21604/diff/1/18?context=10

Which you would invoke as:
  

  DraggableObject obj = new DraggableObject(elem, DraggableObjectOptions.newInstance().setLeft(10).setTop(5));

Can anyone think of a reason I might be shooting myself in the foot performance wise or code size wise as opposed to making these strict setter methods?  I don't want to set a bad example.

-Eric.
--
Eric Z. Ayers - GWT Team - Atlanta, GA USA
http://code.google.com/webtoolkit/

[Vitali Lovich]

No I don't think so.  There might be some additional overhead of returning a value instead of void, but I"m pretty sure that would be extremely negligible to the point of never becoming an issue, regardless how hard you try (the function invocation itself should dominate that by orders of magnitude).

[Ray Cromwell]

Well, for one thing, there appears to be a bug in the compiler.
Consider the following:

public void onModuleLoad() {
Window.alert(Builder.newInstance().setFoo("Hello").setBar("World")
.setBaz("!").toString());
}

static final class Builder extends JavaScriptObject {

protected Builder() {
}

public static Builder newInstance() {
return createObject().cast();
}

public native Builder setFoo(String x) /*-{
this.foo = x;
return this;
}-*/;

public native Builder setBar(String x) /*-{
this.bar = x;
return this;
}-*/;

public native Builder setBaz(String x) /*-{
this.baz = x;
return this;
}-*/;
}

The output from this is:

$wnd.alert($toString($setBaz($setBar(({}.foo = 'Hello' , {}), 'World'), '!')));

The multi-expression that is inlined in place of setFoo() does not
yield the right 'this' value.

You can workaround this bug by preventing the JsInliner from inlining,
you can do this in several ways, one way is to violate evaluation
order, e.g.

public native Builder setFoo(String x) /*-{
return x=this.foo=x, this;
}-*/;

This forces 'x' to be evaluated before 'this$static' which is the real
first parameter of this method, so inlining is derailed. The
generated code then looks like this:

$wnd.alert($toString($setBaz($setBar($setFoo({}, 'Hello'), 'World'), '!')));

function $setFoo(this$static, x){
return x = this$static.foo = x , this$static;
}

In an ideal world, we'd have something like this generated as JS:

DraggableObjectOptions.newInstance().setLeft(10).setTop(5)

becomes

{left: 10, top: 5}

One way to accomplish this to have your builder use 'eval' and String
concat, e.g.

static final class Builder extends JavaScriptObject {
protected Builder() {}
public static native Builder newInstance() /*-{
return "{";
}-*/;

public native Builder setFoo(int x) /*-{
return this + "foo:"+x+", ";
}-*/;

public native Builder setBar(int x) /*-{
return this + "bar: "+x+", ";
}-*/;

public native Builder setBaz(int x) /*-{
return this + "baz: "+x+", ";
}-*/;

public native Builder end() /*-{
return eval(this + "}");
}-*/;
}

(hosted mode version not shown)

With proper static simplification in JS, something like
setFoo(10).setBar(20).setBaz(30) is compiled to:

eval("{foo:10,bar:20,baz:30}")

Of course, eval is yucky to use.

-Ray

[Vitali Lovich]

Oh, I didn't realize there was a compiler bug regarding this.  Is this an issue regarding returning this from native?

Builder setFoo(String s)
{
    setFooImpl(s); // native function that does the actual code
    return this;
}

fix the issue?  More code but the inlining compiler should produce the same optimized code here except correct (although annoying having to maintain both interfaces to workaround the bug).

Alternatively, in the newInstance, if you initialize all the variables in your class to undefined, then it'll fix this issue (although you're object may be bigger than necessary), no?

[Isaac Truett]

Personally I don't care for this style of coding. I think it's very
convenient for writing, but it hurts readability.

On Thu, Apr 16, 2009 at 2:03 PM, Eric Ayers <zun...@google.com> wrote:

[Ray Cromwell]

I think it depends on formatting, I find something like:

Builder.create().
foo("Hello").
bar("World").
baz("!").
end();

to be just as readable, if not more so than

Builder b = Builder.create();
b.setFoo("Hello");
b.setBar("World")
b.setBar("!");

Not just because it's less typing and repetition, but because the
repeated boilerplate and non-indentation obscure the hierarchy. This
is one area where I like Python, Haskell, YML, etc is that structuring
the code visually aids in rapid understanding of relationships.

-Ray

[Eric Ayers]

I agree you can write ugly code this way, but since its an API for others to use, I'm not trying to make that kind of judgement, just make it possible for others to code in that style if they want. 

I will note that I did find it very helpful when having to invoke super() in a constructor, or setting just one argument on the object as a function parameter and then throwing it away.