Closed jGleitz closed 3 years ago
jGleitz
commented
5 years ago Oh, by the way, why does ExpectImpl not directly delegate to the val that does the loadSingleService call? What is the purpose of the builder objects? They sometimes contain interfaces, but those could also be moved to the api-projects, could they not?
robstoll
commented
5 years ago @jGleitz Thanks for opening this discussion. I think it is worth discussing this and I am open to change things as I have ask myself such question also quite many times and we are not yet at 1.0.0 and I always said that there might be breaking changes in core/domain. Or in other words, we are relatively free but of course, I would try to make most things backward compatible.
Maybe first a bit background to understand why things exist:
AssertImpl now ExpectImpl. However, nowadays it provides more than just ExpectImpl, it provides further simplifications and guidance for the user or in other words less abstract constructs. Yet without the possibility to exchange it (in the end it only delegates to core where the exchange would need to happen).Another benefit of the modular design is, well, modularity, separation of concerns, open/close principle and the like. It allows to hide things from other modules.Theoretically it also allows to release them independently but I doubt this is ever going to happen.
delegation Using delegation only is not possible as the builders provide more than the implemented interfaces. In other words, your example is a bit overly simplified :) I opened a feature request some time a go for delegation with inline behaviour, see https://youtrack.jetbrains.com/issue/KT-23241. Using inline has no great effect on performance but on the API as the user effectively consumes the interface and not the builder, allowing to change things more easily without backward compatibility issues. As the domain-builder serves mainly as delegation, IMO this makes sense and outweighs the lines we need to write in addition (yet delegation with inline behaviour would still be great).
Inversion of control Using a dependency framework would not replace domain-builders as domain-builders module is not responsible for the inversion of control but the ServiceLoader mechanism is. We could replace ServiceLoader with a dependency framework though but... ServiceLoader is very fast compared to other DI-frameworks and does not bring any dependencies, its already in Java (since 6). However, things look different with JS and other platforms. The current implementation in JS is also, just a DI framework without dependencies.
I agree that it is not very likely that one will use different assertion function implementations. It is more likely that one will provide an additional function instead. Maybe even one which might do almost the same thing as an existing function but a bit different. I guess that is more likely to happen as it is easier to implement. I am glad you raise the discussion as I though myself already many times if I should reduce the complexity here (not for the end user but mainly to ease the path for contributors).
So I guess the question boils down to, why do we need inversion of control or rather what do we want to achieve with it? The question arises what happens if we don't particularly like a certain implementation in the sense of that we don't want to maintain it? People should have the possibility to use own Reporters etc. Ergo, a very rough use case:
as a user of Atrium, I want to be able to use my own implementation for certain parts without the need to rewrite Atrium, so that I can achieve a different behaviour
There are different ways of achieving such a goal, two are:
where configuration and inversion of control lay close to each other and are many times the same thing (just different naming). So maybe we should make the distinction between static (hard coded in code) and dynamic (configuration files such as yml, xml or ServiceLoader) inversion of control/configuration => I'll stick with configuration in the following section as it is shorter :grin:
Let us first look what parts can already be dynamically or statically be configured:
Reporter either static ReporterFactory.specifyFactory... or dynamic ReporterFactory via ServiceLoader.Now, let's look at each module and see if we need dynamic or static configuration.
A reporter incorporates quite a bit of configuration and is usually created via ReporterBuilder. Following an example: https://github.com/robstoll/atrium/blob/master/apis/cc-de_CH/atrium-api-cc-de_CH-jvm/src/test/kotlin/ch/tutteli/atrium/atriumVerbs.kt#L45
Do we need static configuration? definitely yes Do we need dynamic configuration? depends
A dynamic configuration was introduced so that one can define a different Reporter than the default one in case the built-in assertion verbs are used. We could say that one has to use custom assertion verbs in order to define a different Reporter.
In which scenarios do we need dynamic configuration with built-in verbs? Imagine you use Atrium for quite a long time, you have always been happy about the default configuration. At some point the maintainers decide to introduce a coloured console output (on my waiting list btw) with a shitload of fancy emoji (not on my waiting list) which you don't like at all. If you can just re-configure the Reporter dynamically, then you can do the configuration most likely in one place without touching test code. If you need to go via custom assertion verb, then you need to introduce it and replace an import. So what do you think: is it worth to have a dynamic configuration for such use cases?
The core factory provides things like:
I am not going to analyse each of them, that would be too much now. What I want to point out though, is that we already provide different behaviour for interfaces within this CoreFactory. For instance, I already implemented two different kinds of AssertionPairFormatter we could provide further kinds if required. One is already able to configure them via ReporterBuilder
Do we need the dynamic configuration? probably not
Most components can be configured statically when defining the Reporter. The need to exchange the CoreFactory is only there for a similar scenario described in the reporter section. Yet, if we can define the Reporter dynamically, then there is only ReportingAssertionContainer, CollectingAssertionContainer and AssertionChecker which cannot be exchanged. But they are kind of the center piece of Atrium, I doubt someone would want to exchange them.
=> conclusion: IMO we could reduce core-robstoll and core-robstoll-lib to one module core-impl. I would stick with core-api though for modularity reasons. What do you think?
IMO they fall into the same category as CoreFactory.
Things like AnyAssertions etc. having an interface makes sense as it makes sure that the builders in domain-builders have the same signature. But...
Do we need dynamic configuration? depends
Always a good answer, depends. I guess in most cases not really unless you would want to provide further failure hints to the users which we as maintainer don't want to include in Atrium. Following a scenario: the project where Atrium has to deal a lot with dates. If an assertion fails due to date comparison, then it is many times due to things like: it is the last day of the month, quarter or year, the test started at 23:59 and suddenly there is another day etc. To minimise the time a developer has to invest to look why a certain test in the CI failed, a hint checking such scenarios and spitting out the hint is very valuable (also monetary wise).
The question is now, should such a company exchange DateAssertions (Atrium does not yet have any, but they could be valuable for scenarios like the above, providing extra hints -- so far no one asked for it though and I don't have that scenario currently) with an own implementation or should they implement custom assertion functions?
What do you think?
IMO it should be possible to exchange them statically (by changing the jar) and also dynamically (via TranslationSupplier)
jGleitz
commented
5 years ago Thank you for your elaborated response. I really appreciate you taking the time to answer in-depth. I sense that this will be a long thread, but also one having, especially because the project has not yet reached version 1.0.0.
TL;DR:
Maybe some feedback at the start: I have read the docs, I scanned through the code, I have read your comments. I am still not sure which part of functionality belongs where. So at least for me, the architecture feels unnecessarily complicated. Here is what I understand the purpose of, and what I do not understand:
atrium-domain-api :heavy_check_mark: atrium-domain-robstoll: :question: what is the difference to atrium-domain-robstoll-lib?atrium-domain-robstoll-lib: :question: what is the difference to atrium-domain-robstoll?atrium-domain-builders :question: looks to me like a mixture of api and non-api, this is confusing.Areas where I still have questions:
domain-builders served initially one purpose, give the user one single point of entry when it comes to more complex assertion […] it provides further simplifications and guidance for the user.
Wait, do you consider domain-builders part of the public user-facing API? My assumption was that only the api projects (atrium-api-*) are meant to be used by users in tests (and I would also advocate this principle!).
why
*-robstolland*-robstoll-lib? domain-robstoll is quasi an abstract factory, providing implementations for domain but without exposingdomain-robstoll-lib. You could exchange this abstract factory with another one if you like.domain-robstoll-libon the other hand can be reused by other implementations but without backward-compatibility guarantees.
I do not get it. Under which circumstances would exchanging the abstract factory make sense? If all the code was in one module, implementors wishing to implement a different domain (or core) could still reduce parts at their own risk.
The way you have defined “dynamic configuration”, I understand it as “can be changed without recompiling the user’s test code”. I do not think that this is a use case at all. I do not think anybody is reusing compiled test code and just changing configuration parameters.
However, the way you discuss “dynamic configuration”, it sounds to me more like “can be changed without recompiling atrium”. This will often be a use case.
The difference is this: In the first case, we need more complicated mechanisms (like ServiceLoader). In the second case, a global configuration function is sufficient.
I will not use your terms of “dynamic” and “static” configuration because I am not sure that we share an understanding of them.
I agree that users should be able to exchange these parts without having to recompile artium:
Using inline has [means] the user effectively consumes the interface and not the builder, allowing to change things more easily without backward compatibility issues. As the domain-builder serves mainly as delegation, IMO this makes sense.
I am reading it as “I do not want to have users’ code having byte code references to the builders, only to the interfaces.”. And yes, something like ExpectImpl.path.exists(this) is compiled down to pathAssertions.exists(this). But why the double delegation through ExpectImpl.paths and PathAssertionsBuilder#exists? Why not just pathAssertions.exists(this) in the first place?
If discoverability is the argument here, we could still have ExpectImpl.paths pointing to pathAssertions directly. But I still would not think that it is an improvement.
Some arguments that I find worth mentioning:
As we are talking architecture, I want to point out my personal approach to it. I see it as balancing a lot of contradicting goals. And, on top, I do not think that there are any goals that have any value per sé. So I cannot quite agree with:
Another benefit of the modular design is, well, modularity, separation of concerns, open/close principle and the like.
I, personally, have come to the conclusion that those goals should not be valued on their own. They always contradict other goals, like simplicity a small codebase. So we have to decide case-by-case which goal is more important.
Note also that separation of concerns is usually cited as
gather together things that change for the same reason, separate things that change for different reasons
This project does a very good job in separating things. But adding a single matcher leads to six or more files being touched. Therefore, I feel that it is not sufficiently “gathering together things that change for the same reason”.
robstoll
commented
5 years ago First, I'll try to bring light into what I meant with dynamic and static configuration. I guess you will see things differently afterwards and I'll wait for your feedback again.
I was looking at configuring Atrium from a user/consumer point of view, not a maintainer. Or in other words: Atrium is already compiled and the user gets the jars from bintray/jcenter. I'll try to back this viewpoint up with two examples.
I think the following is more obvious than in the other cases why we need to support a dynamic configuration mechanism (it is another question if we want to support such a use case):
As a user of Atrium, I want to be able to reuse the built-in assertion verb
expectbut still be able to report all assertions and not only the failing ones.
Atrium only has one Reporter currently OnlyFailureReporter. If the user want to use another one, then he needs to provide another class implementing Reporter. This one is dynamic in the sense of: I as a maintainer of Atrium do not know what the user will use to build the reporter, the class as such is not yet defined when Atrium was compiled. So I need to provide a way to dynamically load the new functionality.
I hope it is now clearer what I meant with dynamic. Let's look at the static counterpart:
As a user of Atirum, I want to be able to reuse the built-in assertion verb
expectbut still be able to exchange the bullet points used in reporting.
This one is static as we already know the type which is used for other bullet points, namely String in this sense we don't need a dynamic configuration mechanism but it is good enough if the user can call a function.
Oh man... the scales just fell from my eyes when I re-read the above. We do not even need a dynamic configuration for the first use case as long as we provide a static way to provide another instance of Reporter. We only need it if we want to allow that the user can exchange the reporter even after the user compiled its code.
I do not see the use case for changing parts of atrium without recompiling the user’s test code.
I guess this is heavily influenced by the misunderstanding. Therefore, I'll only comment on the literal aspect of this sentence. What you literally wrote is that Atrium does not need binary backward compatibility (and probably implying that source backward compatibility is enough). That being said, maybe you meant something else with the sentence but let me comment on binary backward compatibility nonetheless. I want binary backward compatibility as it allows to speed up test code in certain circumstances quite a bit (no need to recompile test classes).
That's that, I'll wait for your response.
jGleitz
commented
5 years ago Oh man... the scales just fell from my eyes when I re-read the above. We do not even need a dynamic configuration for the first use case as long as we provide a static way to provide another instance of
Reporter.
Yes! This is the conclusion I wanted to point out.
The other citation (the third part in your comment) was just my poor wording trying to point at this fact.
robstoll
commented
5 years ago I can already shed some more light on other topics:
I see roughly three Personas using Atrium:
Developer which uses only built-in functionality provided by Atrium's API => new assertion functions are only created by composing other assertion functions. A new user of Atrium or a purist => let's call this persona Newcomer. This user would come up with something like:
fun <T: Date> Expect<T>.isBetween(lowerBoundInclusive: T, upperBoundExclusive: T) =
isGreaterOrEquals(lowerBoundInclusive).and.isLessThan(upperBoundExclusive)Developer which uses Atrium but also invents new simple assertion functions. A user which is Atrium already for a longer term => Let's call this persona Long-term User. This user would come up with something like:
fun Expect<Int>.isEven() =
createAndAddAssertion("is", RawString.create("an even number")) { it % 2 == 0 }Developer which uses Atrium but also invents more complex assertion functions for own types. Could be a library author or someone which actually want to understand Atrium fully to get most out of it => Let's call this persona Library Author This user would come up with something like:
fun <A, B> Expect<Either<A, B>>.isLeft(): Expect<A> = changeToLeft().getExpectOfFeature()
fun <A, B> Expect<Either<A, B>>.isLeft(assertionCreator: Expect<A>.() -> Unit) =
changeToLeft().addToInitial(assertionCreator)
private fun <A, B> Expect<Either<A, B>>.changeToLeft(): ChangedSubjectPostStep<Either<A, B>, A> {
return ExpectImpl.changeSubject.reportBuilder(this)
.withDescriptionAndRepresentation("is a", RawString.create("Left"))
.withCheck { it.isLeft() }
.withTransformation { (it as Left).a }
.build()
}ExpectImpl.builder in addition, which means also the domainExpectImpl.changeSubject, maybe also extend current builders (ignoreCase for CharSequence.contains could be seen as such a case) or even provide own implementations for core components.In this sense Atrium provides three API level with different needs: 1. API, 2. domain, 3. core. The only stable so far is the 1.
I see what you mean. I guess the missing piece is this. I constrained most services to 1 implementation at runtime so that a) I can cache them and b) for reasons like avoid shaky tests due to alternating implemtations. Which means, if a user wants to use a different core-api implementation then one needs to exclude core-robstoll and provide an own abstract factory. This abstract factory can reuse implementation of core-robstoll-lib though.
But anyway, to cite myself:
Most components can be configured statically when defining the Reporter. The need to exchange the CoreFactory is only there for a similar scenario described in the reporter section
And I have already realised that the reporter does not need to be dynamic. So my conclusion stands:
IMO we could reduce core-robstoll and core-robstoll-lib to one module core-impl. I would stick with core-api though for modularity reasons. What do you think?
I guess this point should be clear now (I actually overlooked this section before)
Assertion contracts where we want to support the exchange of the underlying implementation or if we say, nope, an long-term user would just write an own assertion function.It is mainly about discoverability, I think this is one thing which lot of libraries do wrong. Having one entry point reduces the cognitive load a lot.
Internally we could use pathAssertions.exists(this) directly but I think there are enough users looking at the implementation of Atrium to find out how they should do it themselves and thus it makes sense to write ExpetImpl.path instead.
Yet, it is also about extensibility. A library author can provide additional assertion function on existing types without the need to provide an own construct. Which means, the library author does not have to explain something to the user and the API is kind of intuitive to use.
Also if a library author provides assertion for new types, they are easily discoverable on a domain level (no need to know package names etc.)
jGleitz
commented
5 years ago Wow. Your first paragraph gave me a lot of insight. Thank you! Maybe we should even persist these three personas in the Wiki or something. It helped me a lot.
- Why delegation?
I see. I think you make a good point there! (Athough it only justifies one step of the two-step delegation).
I would like to take a turn and make a concrete proposal how we could re-structure atrium. It is meant as basis for discussion and as a check for what I have understood by now—and what not. Give all I have learned in this thread, I would currently assume this to be a good architecture for atrium:
assertion-builder module (divided into API and implementation), that provides all the capabilities of creating assertions as well as reporters. Classes like ReporterBuilder, AssertionBuilder and so on would go there.assertions module (divided into API and implementation), that provides the actual assertions. Classes like AnyAssertions would go there.Note: maybe it might make sens to take the reporters into their own module, too. I do not know how inter-dependent they are.
The APIs would depend on the assertions module (and maybe the assertions-builder module, although it would be worth exploring whether we can avoid that). The assertions module would depend on the assertions-builder module.
The latter two modules would both have one entry point object, although this object would not be the same. They could for example be named AssertionBuilder and AssertionImpl. Currently, ExpectImpl is the entry point for both the abstract assertion builders and the concrete assertions. I think this is irritating.
The entry point objects would serve two purposes:
AnyAssertions, users could easiliy just exchange the pointer to change implementationsThe latter might look like this:
class MyAnyAssertions(private val delegate:AnyAssertions) : AnyAssertions by delegate {
override fun <T : Any> toBe(subjectProvider: SubjectProvider<T>, expected: T) = /* my custom implementation */
}
/* Somewhere in the test setup code */
AssertionsImpl.any = MyAnyAssertions(AssertionsImpl.any)The way I imagine it, there would be no more dynamic loading, everything would be adapted using a pattern like the one I have sketched above.
What do you think about this proposal? What have I gotten wrong? What would work worse than it currently does?
The main advantage would be that it is a lot easier for my head to understand what belongs where. There would also be significantly less delegation.
robstoll
commented
5 years ago am afraid but at a first glance I don't like the suggested separation of the modules. At least not yet, I don't see benefits of doing so but that might just be that I don't see what goals you want to achieve.
On the other hand, I am pretty sure I don't want to see the pattern you suggested. I am with you to say the user could overwrite AnyAssertions the way you showed though. What I don't want is mutable state. We will run into hairy situations when it comes to parallel test execution. The way to go IMO is having the assertion-verbs as entry point for any customisation. They form a good level of granularity as one could customise each expect(...) (built-in verbs) but could also create own assertion verbs and use them project wide (and of course adjust them in places where needed).
Which means, the user should not have only the possibility to adjust Reporting (as currently) but also adjust the assertion implementations, and any other component we want to make interchangeable.
I propose we start of with writing down the requirements we want and conclude then how the architecture should look like. I created a Wiki page for this purpose:
Please continue there, writing down all requirements you would like to see fulfilled by Atrium. I intentionally only wrote down one requirement so that you are not biased by my agenda. This will give me good insights what you want and is probably the best way to see how Atrium should evolve.
ExpectImpl is the entry point for both the abstract assertion builders and the concrete assertions. I think this is irritating.
In what way is it irritating? The concrete assertions as well as ExpectImpl.builder... or ExpectImpl.collector build assertions in the end. But maybe you are referring to the ReporterBuilder? Personally I already thought to move reporterBuilder, coreFactory out of ExpectImpl but kept it there as I could not come up with another name. This two and the new introduced assertionVerbBuilder are kind of misplaced and don't really belong to ExpectImpl. So I am happy to hear an alternative solution.
One additional note why I introduced -robstoll and -robstoll-lib + ServiceLoader in the first place -- maybe I should have written down the requirements I had initially; I cannot remember everything, one came to my mind just now. One initial goal I had for Atrium was that the resulting should be relatively small. This especially means:
In order to be able to do this it means: the consuming jar (e.g. an API) is not allowed to reference anything of the implementation (otherwise you cannot exclude it from the classpath). This lead to the design to have an abstract factory which can be exchanged. First there was just one -impl jar which contained the abstract factory, making the abstract factory part of the API. That was a bad decision for one particular reasons. It meant that exchanging the jar required that the other jar had the duplicate class at the same package => this violated jdk9 modularity. Hence the introduction of ServiceLoader to really decouple the API from the core implementation.
jGleitz
commented
5 years ago First of all: Thanks a lot for your patience to discuss all of this! I expected something like “this is getting too much, we will leave everything as it is” halfway through the discussion. Awesome! :+1:
I propose that we collect requirements over in the wiki, but continue discussing them here.
What I don't want is mutable state. We will run into hairy situations when it comes to parallel test execution.
Got you. I will add this to the requirements.
I don't see benefits of doing so but that might just be that I don't see what goals you want to achieve.
Totally understandable. I just wrote down my conclusion, not my reasons. Sorry!
I propose to separate the abstract assertion builders from the assertion separation because I think they have different use cases:
toBe, another one re-uses toBe. The second one should then adopt the modification made by the first one).So one module contains logic that users can depend directly on, the other contains logic which nobody must directly depend on. Note: By that logic, reporters should also be their own module.
I recognise, though, that my proposal is not the only solution for that goal. I think it separates the concerns very well because for me the abstract builders are a very different concern than the assertion implementation.
allow the user to exchange an implementation and thus exclude it from the dependencies to keep the dependency-size (in bytes) small
I do not buy this one. I think it very unlikely that somebody will remove a whole implementation jar. Because that would mean that they have replaced the whole implementation of atrium. If they do that, they maybe should not use atrium in the first place.
I think it is likely that someone wants to replace some part and leave the rest to the existing implementation. I cannot see why it is a use case to remove a whole jar.
jGleitz
commented
5 years ago Maybe some background to why I propose to change anything in the first place:
I have tried to understand how atrium is built and it was a lot more involved than I expected it to be. There are some good reasons for some of the unexpected things, but I nevertheless feel that other areas can be simplified.
On top, the project contains a considerable amount of customised build logic. In my (limited) personal experience, build logic is especially difficult to evolve and a maintenance burden. (Most of my experience on that one comes from complicated build logic I have written myself!)
Now all of this could be a problem that is specific to me. However, I flatter myself to think that I do not have under-average skills in reading and understanding code and architectures. So I expect that any simplification would make it easier for others to contribute. It would at least make it easier for me.
robstoll
commented
5 years ago I propose that we collect requirements over in the wiki, but continue discussing them here.
fine with me
I think it is likely that someone wants to replace some part and leave the rest to the existing implementation. I cannot see why it is a use case to remove a whole jar.
I actually forgot to write that I do not longer see this as a requirement myself. One can still use protractor or similar if size is really a concern.
On top, the project contains a considerable amount of customised build logic
Something we can improve as well if you like but maybe one thing at a time, something for a different issue I would suggest.
So I expect that any simplification would make it easier for others to contribute. It would at least make it easier for me.
That's perfectly reasonable and I share the concern that things are too complicated for new contributors. I hope though that the entrance into the project was still pleasant, if we need to improve there, then please share your thoughts as well.
Back to the shared concern that there are things which aren't really necessary needed (more a nice to have) but make things way more complicated, especially for new contributors. It's not the first time I think I should maybe drop this or that. I already started to pave the way for certain things, for instance:
And frankly, I thought more than once if we should maybe get rid off different API styles and only provide fluent because this introduces also a lot of maintenance and complexity. Yet, so far I figured why not keep it, it's kind of OK. However, now that we switch to from Assert to Expect with 0.9.0, I thought many times, maybe we could at least drop de_CH, because it was more of a prototype to see how well we could potentially write the API in a different language which in the end is not different from a different style. So it would be enough to keep infix as a second style to prove this. (Edit I made up my mind now, I'll drop the API, see robstoll/atrium#137)
That being said, some of the architectural designs I came up with were sometimes intentionally a bit more complicated just to see if they justify their benefits. It's perfectly reasonable to do a refactoring here as well :slightly_smiling_face: and your inputs are very much appreciated :+1:
robstoll
commented
5 years ago @jGleitz I saw you already added some requirements in addition. Let me know once you have written down all requirements which come to your mind. And do not restrict yourself to things we discussed here, write down all requirements which you expect from Atrium.
jGleitz
commented
5 years ago Circling back on this:
robstoll
commented
5 years ago I still do not understand the difference between the domain and the assertion implementation.
publish and module confuses me). Could you try to re-phrase it or provide an example? Maybe we should come up with a glossary as well? For me a module in the context of Atrium maps more or less to a jar but independent of the platform (e.g. atrium-core-api)
I have added R8 and R9 and will add more later on.jGleitz
commented
5 years ago I suggest that once we have a consent as here we create the ticket back in Atrium. Agreed?
Yes.
I would also reduce domain-robstoll and domain-robstoll-lib to one module domain-impl => maybe this answers your open question?
Maybe. As I said, I am beginning to lose overview. If we agree that this is a sensible step, let us do it and I will re-assess my understanding afterwards.
I think I now what you want to say with R3 combinable, shareable implementation changes but it is written in a manner I am not 100% sure
Oh I had great troubles wording this one. What I am trying to say is: Other entities (that are not the atrium maintainers themselves) can publish add-ons/implementation change modules.
For example: author A publishes a different implementation for toBe, author B a different implementation for isA and author C a different reporter implementation on maven central. Users should now be able to include all three modules (i.e. via gradle, possibly also by calling some registerPlugin method in atrium) and “it works”.
Does that help or only add more irritation?
I have added R8 and R9 and will add later on.
I really like how R7 and R8 interact with each other. I feel like they give a good definition of what atrium should contain. And they are a good reference for future discussions.
robstoll
commented
3 years ago closing this as domain builders as discussed here don't exist any more
Atrium contains the builder projects, which to my understanding serve as a bridge between the domain-api and the domain implementation (i.e. domain-robstoll). The builder projects consist mainly of delegating classes (see below).
I think that this architecture can be improved, an see several possibilities to do so. The goal of this issue is to start a discussion about that. I have only recently started working on atrium and am not aware of the many, many considerations that have obviously gone into the codebase. It is likely that the response to this discussion will be “I know the arguments, the current solution is the best one”. And this is absolutely fine. I nevertheless want to discuss. Because best case, we can improve the code. And worst case, I (and maybe others) learn something about atrium.
Do we need dependency inversion?
Uncle Bob defines dependency inversion as:
To my understanding, this is the architectural intend in the separation of domain-api, domain-builders and domain-robstoll.
However I cannot see any real advantage of this pattern here. Dependency inversion gives the possibility to replace implementations. But why would I do that?
It obviously is a meaningful choice to separate the API from the actual assertion (domain) implementation: there can be multiple API styles and those styles are prone to change. In contrast, I cannot think of a reason why I would want to keep an assertion API but change the underlying matcher (domain) implementation. There is not much freedom for different implementations of a matcher. So why would I want to create a second implementation?
Architecture is always about balancing different goals. In this case, I think the separation leads to more complicated design and more code while bringing almost no benefit. So one option is to have one implementation of the domain and no separation between interfaces and implementation.
Implement dependency inversion differently
If we decide that the dependency inversion pattern is, in fact, a good choice, we could still implement it differently.
The main issues I see currently is that the builder projects together with the Java ServiceLoader logic and the custom glue code for Javascript create a lot of complexity and code, and thereby maintenance effort.
A: Use dependency injection
The standard solution for dependency inversion is dependency injection. We could use a framework like KodeIn, which would replace the builder projects entirely. There would be no more service loader logic, no more custom JavaScript glue code generation and no builders. Instead, there would only be an (interchangeable!) dependency injection module, which binds interfaces to implementations.
B: Use Kotlin delegation
If we decide that neither removing dependency inversion nor using dependency injection is a good option, we could at least remove a lot of boilerplate code by removing the custom delegation implementation with Kotlin’s Implementation by Delegation.
The only “drawback” I see here is that the implementation functions would no longer be
inline. However, from my understanding, the performance improvement ofinlineis neglectable in this case. Because any performance improvement (not creating a lambda object, not creating a generic bridge) will be removed in the next call because we call a non-inline method on an interface.Code Example: This long class:
can be replaced by one line of code: