Code evolution with constructor chaining #1995
Description
Hi, while working on PR #1992 / Issue #1629 I have extended some classes by constructor chaining. (It looked like the way to go ;) )
util/src/main/java/io/kubernetes/client/informer/cache/Controller.java
public Controller(
Class<ApiType> apiTypeClass,
DeltaFIFO queue,
ListerWatcher<ApiType, ApiListType> listerWatcher,
Consumer<Deque<MutablePair<DeltaFIFO.DeltaType, KubernetesObject>>> processFunc,
Supplier<Boolean> resyncFunc,
long fullResyncPeriod)
public Controller(
Class<ApiType> apiTypeClass,
DeltaFIFO queue,
ListerWatcher<ApiType, ApiListType> listerWatcher,
Consumer<Deque<MutablePair<DeltaFIFO.DeltaType, KubernetesObject>>> processFunc,
Supplier<Boolean> resyncFunc,
long fullResyncPeriod,
BiConsumer<Class<ApiType>, Throwable> exceptionHandler) {
util/src/main/java/io/kubernetes/client/informer/impl/DefaultSharedIndexInformer.java
public DefaultSharedIndexInformer(
Class<ApiType> apiTypeClass,
ListerWatcher<ApiType, ApiListType> listerWatcher,
long resyncPeriod) {
public DefaultSharedIndexInformer(
Class<ApiType> apiTypeClass,
ListerWatcher<ApiType, ApiListType> listerWatcher,
long resyncPeriod,
Cache<ApiType> cache) {
public DefaultSharedIndexInformer(
Class<ApiType> apiTypeClass,
ListerWatcher<ApiType, ApiListType> listerWatcher,
long resyncPeriod,
Cache<ApiType> cache,
BiConsumer<Class<ApiType>, Throwable> exceptionHandler) {
public DefaultSharedIndexInformer(
Class<ApiType> apiTypeClass,
ListerWatcher<ApiType, ApiListType> listerWatcher,
long resyncPeriod,
DeltaFIFO deltaFIFO,
Indexer<ApiType> indexer) {
public DefaultSharedIndexInformer(
Class<ApiType> apiTypeClass,
ListerWatcher<ApiType, ApiListType> listerWatcher,
long resyncPeriod,
DeltaFIFO deltaFIFO,
Indexer<ApiType> indexer,
BiConsumer<Class<ApiType>, Throwable> exceptionHandler) {
Similar to construction methods in util/src/main/java/io/kubernetes/client/informer/SharedInformerFactory.java
public synchronized <ApiType extends KubernetesObject, ApiListType extends KubernetesListObject>
SharedIndexInformer<ApiType> sharedIndexInformerFor(
ListerWatcher<ApiType, ApiListType> listerWatcher,
Class<ApiType> apiTypeClass,
long resyncPeriodInMillis) {
public synchronized <ApiType extends KubernetesObject, ApiListType extends KubernetesListObject>
SharedIndexInformer<ApiType> sharedIndexInformerFor(
CallGenerator callGenerator,
Class<ApiType> apiTypeClass,
Class<ApiListType> apiListTypeClass) {
public synchronized <ApiType extends KubernetesObject, ApiListType extends KubernetesListObject>
SharedIndexInformer<ApiType> sharedIndexInformerFor(
CallGenerator callGenerator,
Class<ApiType> apiTypeClass,
Class<ApiListType> apiListTypeClass,
long resyncPeriodInMillis) {
public synchronized <ApiType extends KubernetesObject, ApiListType extends KubernetesListObject>
SharedIndexInformer<ApiType> sharedIndexInformerFor(
ListerWatcher<ApiType, ApiListType> listerWatcher,
Class<ApiType> apiTypeClass,
long resyncPeriodInMillis,
BiConsumer<Class<ApiType>, Throwable> exceptionHandler) {
This approach seems to have some disadvantages if we want to keep the constructors for backward compat. As an alternative we could use builders, e.g.
SharedIndexInformerBuilder.builderFor(apiType, apiListType)
.withListerWatcher(listerWatcher) // or else default
.withCallGenerator(callGenerator) // or else default
.withResyncPeriodInMillis(resyncPeriodInMillis) // or else default
.withExceptionHandler(exceptionHandler) // or else default
.build();
Advantages of the builder strategy
- constructor chaining is limited, all must be done on the single this call, builders not
- calling statics during chained constructors is only a workaround - not neccessary with builders
- these classes can grow only because we want to keep the constructors for backward compatibility - construction code is seperated from class responsibility (a good idea if construction code grows)
- clear defaults logic in the builder code
- testing builders is easier than constructors
- you name it
Additionally, classes like DefaultSharedIndexInformer, that create their own dependencies with new are difficult to test. If passing dependencies from outside testability increases.
What's about
- make all constructors of the listed classes with sub sets as deprecated for later removal
- keep the all params constructor, make it package protected, provide a builder for it
- replace all new calls creating dependencies within the constructors with injected dependencies
- replace new calls from within any non constructor methods with injected factories for later object creation
Sure, this has to be applied in baby steps to prevent breaking functionality.
Another option:
Add PowerMock for testing new and statics - but it is not my preferred one, only a last resort.
What do you think about it?