Issue 33 : Added additional constructor for passing an instance of StreamBuilder

[shiv4nsh]

I want to resolve #33 so that the instance of StreamBuilder can be overriden

[shiv4nsh]

@debasishg : I have made changes, please review !

[debasishg]

Thanks for the PR. I will take a look over the weekend and merge.

[debasishg]

Can u please keep the name inner here ? We use this name consistently in other classes as well.

[debasishg]

On concern is that the existing API will break. Those who are using new StreamsBuilderS() will need to change to the object syntax. But I guess this will be a worthwhile change and we are only in 0.1.0. Thoughts @deanwampler, @lightbend/fast-data ?

[blublinsky]

I actually like the previous version more. In the proposed PR we kinda mixing Java and Scala APIs. Any specific reason for this?

[debasishg]

The problem with the earlier version is that the API cannot be used if someone wants to use the Scala API for a mocked StreamsBuilder, say, for unit testing. Or someone may have a specialized version of StreamsBuilder derived from the Java base StreamsBuilder. She cannot use it with the Scala API since we are constructing the base Java StreamsBuilder within the class itself. S, it's always recommended to pass in the instance (constructor injection) instead of creating the hardcoded instance inside.

[debasishg]

Some comments embedded.

[blublinsky]

Then lets introduce an extra constructor for StreambuilderS I really do not want to be forced to create a java object first Boris Lublinsky FDP Architect boris.lublinsky@lightbend.com https://www.lightbend.com/

…

On Jan 12, 2018, at 1:17 PM, Debasish Ghosh ***@***.***> wrote: @debasishg commented on this pull request. In src/main/scala/com/lightbend/kafka/scala/streams/StreamsBuilderS.scala <#34 (comment)>: > } +object StreamsBuilderS { + The problem with the earlier version is that the API cannot be used if someone wants to use the Scala API for a mocked StreamsBuilder, say, for unit testing. Or someone may have a specialized version of StreamsBuilder derived from the Java base StreamsBuilder. She cannot use it with the Scala API since we are constructing the base Java StreamsBuilder within the class itself. S, it's always recommended to pass in the instance (constructor injection) instead of creating the hardcoded instance inside. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYHWGyjTmBdrE_14MmNfO0PjbGah3F9tks5tJ67CgaJpZM4Rb1CH>.

[shiv4nsh]

@blublinsky But normally we are calling StreamBuilderS() directly and do not need to create the Java object, the other apply method is only for passing an already presence instance.
But its totally upto you guys @debasishg , @blublinsky , I am also learning and I want to follow the standards ! Please let me know what changes are needed .

[debasishg]

@blublinsky That's exactly what is done with the companion object. Regarding

I really do not want to be forced to create a java object first

We are not creating the Java object first. If the client invokes val b = StreamsBuilders, then the def apply() in the companion object creates the Java object and passes to the constructor. OTOH the client can also invoke val b = StreamsBuilderS(new StreamsBuilder), which uses the Java object we pass.

Anyway creating an object inside the class using a hard coded constructor is an anti-pattern, since the class loses it's testability. In the current implementation we cannot write a unit test for StreamsBuilderS since the construction of the Java object is hardwired with the class. Separating it as a constructor injection nicely decouples the 2. If we want to write a unit test for StreamsBuilderS we can pass a mocked Java object while constructing the Scala object.

[debasishg]

👍 @blublinsky .. This will not break the current API as well. I was thinking of delegating the construction process to the companion object (something like factory methods or smart constructors). But this approach will serve us good as well. We have been able to pull the construction of the Java object out of the class implementation. Later if required we can add companion object and additional constructors.

[shiv4nsh]

@debasishg : Made the changes as per the new comments please review

[debasishg]

Thanks a lot @shiv4nsh .. I will do a check of the final version over the weekend and do the merge. It's quite late here in India and I don't want to merge half sleepy :-)

[shiv4nsh]

Haha No worries @debasishg . I was just overwhelmed , it was a fan moment for me .
As you and Venkat were the only reason I started learning Scala in the first place from FunctionalConf , India .