Armeria: Using 'builder()' factory method instead of constructor

Created on 16 Apr 2019  路  13Comments  路  Source: line/armeria

Currently, we instantiate a builder by calling a constructor or a static method in a builder class:

Server server = new ServerBuilder()
        .foo()
        .bar()
        .build();

I wonder if it was a good idea or we need to reconsider this decision in favor of having builder() method:

Server server = Server.builder()
                      .foo()
                      .bar()
                      .build();

The pros of builder() are:

  • We can easily align at '.', which is better in terms of aesthetics.
  • We do not need to import ServerBuilder if we chain all.
discussion

Most helpful comment

@4whomtbts It's good to be back! I think you need to know some changes in #2323.
1) Please make sure your current branch is up-to-date from the master branch. Lots of class which have HTTP keyword have been deprecated.
2) HttpAuthServiceBuilder and HttpFileServiceBuilder have been done, you may want to skip them.
3) This issue is scheduled to apply before release 1.0.0. Maybe in the middle of January. 馃槈

I'm looking forward to your next PR. Cheer up!


  • ClientBuilder
  • DefaultClientBuilderParams
  • ServerListenerBuilder
  • ~HttpAuthServiceBuilder~
  • SimpleCompositeServiceBuilder
  • SimpleCompositeRpcServiceBuilder
  • CorsPolicyBuilder
  • CorsServiceBuilder
  • DocServiceBuilder
  • HttpFileBuilder
  • ~HttpFileServiceBuilder~
  • LoggingServiceBuilder
  • JettyServiceBuilder
  • ArmeriaRetrofitBuilder
  • KeyStoreCredentialResolverBuilder
  • SamlServiceProviderBuilder
  • TomcatServiceBuilder
  • ZooKeeperUpdatingListenerBuilder

All 13 comments

I like the Server.builder() way

Let me make the proposed change.

builder() or newBuilder()? Protobuf uses newBuilder() and Guava uses builder(). Which one is better?

+1.001 for builder vs +1 for newBuilder :)

Just came back from Spain and it reads like 1001 vs 1 :rofl:

I'm working on this

The remaining builder classes that need migration:

  • ClientBuilder
  • DefaultClientBuilderParams
  • ServerListenerBuilder
  • HttpAuthServiceBuilder
  • SimpleCompositeServiceBuilder
  • SimpleCompositeRpcServiceBuilder
  • CorsPolicyBuilder
  • CorsServiceBuilder
  • DocServiceBuilder
  • HttpFileBuilder
  • HttpFileServiceBuilder
  • LoggingServiceBuilder
  • JettyServiceBuilder
  • ArmeriaRetrofitBuilder
  • KeyStoreCredentialResolverBuilder
  • SamlServiceProviderBuilder
  • TomcatServiceBuilder
  • ZooKeeperUpdatingListenerBuilder

It has been on progress since this monday. Apologize for for slow progress (__)

@4whomtbts It's good to be back! I think you need to know some changes in #2323.
1) Please make sure your current branch is up-to-date from the master branch. Lots of class which have HTTP keyword have been deprecated.
2) HttpAuthServiceBuilder and HttpFileServiceBuilder have been done, you may want to skip them.
3) This issue is scheduled to apply before release 1.0.0. Maybe in the middle of January. 馃槈

I'm looking forward to your next PR. Cheer up!


  • ClientBuilder
  • DefaultClientBuilderParams
  • ServerListenerBuilder
  • ~HttpAuthServiceBuilder~
  • SimpleCompositeServiceBuilder
  • SimpleCompositeRpcServiceBuilder
  • CorsPolicyBuilder
  • CorsServiceBuilder
  • DocServiceBuilder
  • HttpFileBuilder
  • ~HttpFileServiceBuilder~
  • LoggingServiceBuilder
  • JettyServiceBuilder
  • ArmeriaRetrofitBuilder
  • KeyStoreCredentialResolverBuilder
  • SamlServiceProviderBuilder
  • TomcatServiceBuilder
  • ZooKeeperUpdatingListenerBuilder

@4whomtbts It's good to be back! I think you need to know some changes in #2323.

  1. Please make sure your current branch is up-to-date from the master branch. Lots of class which have HTTP keyword have been deprecated.
  2. HttpAuthServiceBuilder and HttpFileServiceBuilder have been done, you may want to skip them.
  3. This issue is scheduled to apply before release 1.0.0. Maybe in the middle of January. 馃槈

I'm looking forward to your next PR. Cheer up!

  • ClientBuilder
  • DefaultClientBuilderParams
  • ServerListenerBuilder
  • ~HttpAuthServiceBuilder~
  • SimpleCompositeServiceBuilder
  • SimpleCompositeRpcServiceBuilder
  • CorsPolicyBuilder
  • CorsServiceBuilder
  • DocServiceBuilder
  • HttpFileBuilder
  • ~HttpFileServiceBuilder~
  • LoggingServiceBuilder
  • JettyServiceBuilder
  • ArmeriaRetrofitBuilder
  • KeyStoreCredentialResolverBuilder
  • SamlServiceProviderBuilder
  • TomcatServiceBuilder
  • ZooKeeperUpdatingListenerBuilder

Yes. My local branch is currently up-to-date.
Thank you for fill me in!

Once #2352 is merged, these are probably the last:

  • ClientBuilder
  • DefaultClientBuilderParams
  • new SimpleCompositeService(...)
  • new SimpleCompositeRpcService(...)
  • new DocService()
  • new ArmeriaRetrofitBuilder(...)
  • KeyStoreCredentialResolverBuilder

They are somewhat trickier than others, so let me handle them by myself.

Need to replace constants with factory methods as well:

  • EventCount.ZERO
  • ClientCacheControl.*
  • ServerCacheControl.*
  • PrometheusMeterRegistries.defaultRegistry
  • AuthTokenExtractors.*
  • CorsConfig.DISABLED

.. and make non-final public classes final if not meant for extension.

Once #2352 is merged, these are probably the last:

  • ClientBuilder
  • DefaultClientBuilderParams
  • new SimpleCompositeService(...)
  • new SimpleCompositeRpcService(...)
  • new DocService()
  • new ArmeriaRetrofitBuilder(...)
  • KeyStoreCredentialResolverBuilder

They are somewhat trickier than others, so let me handle them by myself.

Ok. I got it.

Was this page helpful?
0 / 5 - 0 ratings