ESchouten
08/06/2021, 6:27 PMjmfayard
08/07/2021, 5:55 PMfun changeOwnPassword()
the supposedly clean architecture wants to force us to have a UseCase, a ChangeOwnPassword noun with an executor property that hides the verb that does all the work.
What happened to functions as first class citizens?
To me it seems like a step back to what Steve Yegge describes in his wonderful article "Execution in the kingdom of nouns" that describes very well why Kotlin is better than Java
http://steve-yegge.blogspot.com/2006/03/execution-in-kingdom-of-nouns.htmlESchouten
08/07/2021, 7:42 PMchangeOwnPassword(args)
kqr
08/07/2021, 9:09 PMESchouten
08/07/2021, 10:06 PMkqr
08/07/2021, 10:43 PMESchouten
08/08/2021, 8:45 AMMarc Knaup
08/09/2021, 12:42 PMsealed class
is more flexible and declarative on the input side.
• Functions cannot contain metadata (needed for inspection/serialization/GraphQL/etc.).
Nothing prevents you however from moving the logic out of the Executor into a plain function somewhere and just call it.The domain and usecases modules doesn’t have any external dependencies.I either misunderstand that one or disagree with it. What exactly is an “external dependency” here? For example common types from external dependencies are fine as part of the domain (e.g. date/time objects,
Url
object, etc.).
You could wrap anything and everything but that has its downsides like plenty of boilerplate and performance & memory suffers.
Also, using an external dependency can help reduce boilerplate code which distracts from the essential: the business logic.Domain module contains all entities, it’s validation and repository interfacesThat one I try to avoid too. I separate the model (entities) from any logic whenever possible. The model is usually shared between server and client which is esp. handy with Kotlin Multiplatform. It can also be shared between multiple standalone parts of server. If everything is merged together then you cannot share the model independently of the underlying business logic (like validation, usually mostly server-side) and low-level access interfaces (repository interfaces). In larger projects it does make sense at some point to evolve and internal model independent from an external model. But that’s overkill in many projects.
LoginUser
doesn’t make sense on a query
. It’s usually a state-changing mutation.
• All fields should start lowercase.
• All those root-level queries and mutations don’t need their own objects. It’s totally fine to have them all in the common roots Query
and Mutation
.
• It’s “logInUser” (verb camel cased) not “loginUser” (noun)
• I’d avoid confusing and meaningless names like a0
. I call them input
(like this: https://github.com/graphql/graphql-js/blob/main/benchmark/github-schema.graphql)ESchouten
08/09/2021, 2:42 PMinterface PasswordEncoder {
fun encode(password: Password): PasswordHash
fun matches(password: Password, hash: PasswordHash): Boolean
}
• Validation in Domain entities
The only validation done at the moment in the domain Entities are field-level validation by the value classes, e.g. URL validness, password meeting requirements. Would that be a no-no for you? By sharing these models I guess these validations would port over.
Fun fact: I started out with Kotlin-multiplatform for the Domain and Usecase module, but found it cumbersome on the Gradle level, thus left it out.
When sharing the Domain module it would make sense to relocate the Repositories to the Usecase module, like @kqr suggested.
Personally I never communicate the Domain Entities to other systems, always use different models/DTO's to prevent returning confidential data like User.password.
• Naming conventions, Query/Mutation
Thank you, noted them all down. Naming in general needs another look here, I'm not confident with several function and variable names.
The a0 input variable (argument-0) is named this way to enable multiple input arguments in the future. Due to Java type ereasure I need to override the KType for the Graphql endpoint generation, and since I am doing this in a generic way the names need to be predictable. Obviously I could rename it to input-0 or even match the Ktypes ordinally with the arguments, which definitely would make it less confusing and meaningless 👍
• DDD
Fully agree with grouping by Domain instead of type. The reason this is not (yet) done like this is the need of sorting into layers in order to enforce the modules access level to other dependencies. E.g. Usecases not knowing of the implementation of the PasswordEncoder, only the interface. By enforce this on a Gradle module level, instead of a developer-mindset level, we eliminate these errors all together.
Ideally I would combine these two ways of grouping, but have no idea on how to do this in a nice manner, do you?
Great, great review, many thanks! I'd love to hear more or hear feedback on the answers I've formulated above!Marc Knaup
08/09/2021, 3:00 PMKType
is fine but anything beyond that should be no-go, esp. if it’s not multiplatform compatible. Most logic can be written without relying on reflection.
Using value classes to hide external dependencies is okay-ish, but has its downside. You may end up wrapping another value class which at least adds a some performance hit. It again can add a lot of overhead that might not make it worth in smaller projects.
I agree that it’s highly dependent on the dependency. Most foundational libraries like kotlinx-datetime should be fine.
A good example on what library I would not like in the Domain or Usecase module is the PasswordEncoder/Validator.I agree. My concern was mainly about the model. Business logic may depend directly on external libs for smaller projects and incrementally abstract those away as the project grows. On a side note: Abstracting away the database for example is something you see quite often (think Hibernate & co). I really hate that approach. It adds incredible complexity and prevents the use of much database-specific functionality. Also, database libraries tend to be complex with big API surface so things like Hibernate are just re-inventing the wheel here. For me it’s usually a repository interface + an implementation that directly uses the database’s own library.
The only validation done at the moment in the domain Entities are field-level validation by the value classes, e.g. URL validness, password meeting requirements.Those two examples are completely separate things for me. A valid URL is part of enforcing a correct datatype. Like having an
Int
value that’s not out of Int
range.
A valid password is high-level business logic that even may change over time. That shouldn’t be in the data model. I once had that with other fields and some kind of max-length. That quickly broke when rules changed over time and the database wasn’t able to load “old” objects that would no longer satisfy new rules.
My Password
value class is merely a wrapper around a String
(can even be empty) with a toString()
that obfuscates the actual password to not log it accidentally.
What constitutes a valid password can be very complex logic, e.g. checking against special password tables etc. Not part of model.
I don’t even validate for PW length on the client side. Client sends to server for validation. Single source of truth.
prevent returning confidential data like User.passwordThat would never happen in a good abstraction. a) You never save the password. You hash it. b) You should separate the user model from the identity model used by authentication. Identity/authentication is purely internal to the server and never part of the shared model.
Due to Java type ereasure I need to override the KType for the Graphql endpoint generation, and since I am doing this in a generic way the names need to be predictable. Obviously I could rename it to input-0 or even match the Ktypes ordinally with the arguments, which definitely would make it less confusing and meaningless 👍That’s why I’ve started my own Kotlin-first multiplatform GraphQL library 🙂 https://github.com/fluidsonic/fluid-graphql (highly experimental, screenshot of example attached, although the code involves another library that does high level abstraction over my GraphQL lib as it doesn’t support that yet) I’ve also attached an example GraphQL schema generated by my libraries.
By enforce this on a Gradle module level, instead of a developer-mindset level, we eliminate these errors all together.
Ideally I would combine these two ways of grouping, but have no idea on how to do this in a nice manner, do you?I do separate by Gradle module and it was super helpful to find many code quality issues. I’m not sure what you mean by the other mindset. How would you separate it and where are the difficulties making it work with Gradle?
ESchouten
08/09/2021, 4:10 PMMarc Knaup
08/09/2021, 4:14 PMI meant User.passwordHash. However, countless other examples could be made-upYup. Hence the separate model object for such data. In a larger project the authentication data is located in a completely separate back-end than the user object anyway (standalone auth server).
What I meant was, when having a module per domain instead of layer, you can’t control the layers accessing other layers which they should not.Ah yes, you could mix update business logic and database logic for example. It’s difficult to separate that further without introducing more modules. Maybe different source sets would be an option? 🤔 There’s gotta be a compromise somewhere.
ESchouten
08/09/2021, 5:10 PMMarc Knaup
08/09/2021, 5:11 PMESchouten
08/09/2021, 5:12 PM(standalone auth server)Which would be a good usecase for domain based modules