the linter and a human reviewer told me there are ...

# codereview

[Thomas Martin]

the linter and a human reviewer told me there are too many params. I am not convinced introducing a data class adds readability or maintenability.
What would be the arguments to reduce param number given kotlin support of named parameters and default values ?

[Blaž Vantur]

Since the method name is called build and some of the parameters in method are expected to be null, could you use Builder Pattern for your use case?
Like it is presented in this article for example.
https://medium.com/@ssvaghasiya61/builder-design-pattern-in-kotlin-50d6c669675c

[Thomas Martin]

Yeah I know builder patterns, while I used it extensively in Java, I do not see value in kotlin, given kotlin support of named parameters and default values.
Builder patterns in kotlin looks like boilerplate to me.
But thanks for the suggestion

[Blaž Vantur]

If there is something I am missing from that method signature are default values for method parameters. If those would be there, we could say that it somehow similar to build pattern. Now, you need to specify each parameter every time you want to call the build method.

In my opinion there is also too many parameters in that method and I would prefer having a build pattern for it. And I would also not go with additional data class for it.

[CLOVIS]

I think the code as it is is fine. Many people would prefer a data class here, but honestly I don't like having to navigate to another file to see what the actual arguments should be.

If the same arguments are specified with multiple functions, it would be a nice use-case for dataargs, but they're not in the language yet.

[Klitos Kyriacou]

Why isn't DossierForSearch a data class already? Then you wouldn't need such a function.

[Thomas Martin]

DossierForSearch is a data class with about 20 params. It takes composite info from this input.
Builder job is to take gross input data and compute necessary info from it to instantiate a DossierForSearch

[Thomas Martin]

one could argue you could write this logic into a secondary constructor of DossierForSearch, but I prefer not to have too much logic into data classes that bind to a database

[Shaun Burch]

As some appear to be optional and others can be empty collections, could use defaults to decrease signature footprint in use?

fun build(
    dossier: DossierOutput,
    workspace: WorkspaceOutput,
    clock: Clock,
    owner: TalentOutput? = null,
    account: AccountOutput? = null,
    parentAccount: AccountOutput? = null,
    project: ProjectExtended? = null,
    activities: Collection<ActivityOutput> = emptyList(),
    tasks: Collection<TaskOutput> = emptyList(),
): DossierForSearch

// Minimal
DossierForSearchBuilder.build(
    dossier = dossier,
    workspace = workspace,
    clock = clock,
)

Or group some associated params into smaller data classes? (Hard to infer from signature, but hopefully illustrates)

fun build(
    clock: Clock,
    outputs: Outputs,
    project: ProjectExtended? = null,
): DossierForSearch

data class Outputs(
    dossier: DossierOutput,
    workspace: WorkspaceOutput,
    owner: TalentOutput? = null,
    account: AccountOutput? = null,
    parentAccount: AccountOutput? = null,
    activities: Collection<ActivityOutput> = emptyList(),
    tasks: Collection<TaskOutput> = emptyList(),
)

DossierForSearchBuilder.build(
    clock = clock,
    outputs = Outputs(
        dossier = DossierOutput,
        workspace = WorkspaceOutput,
    ),
    project = project
)

[Thomas Martin]

Yes default values are a possibility, linter does not care though. I can obviously group some associated params into smaller data classes, my question is why ? Is it really better code ? Or is it juste a legacy rule for programming languages that did not support named parameters and default values that we should ditch ?
I'm leaning for the latter, but wanted to read arguments I could have missed.

[Thomas Martin]

found a quote in effective kotlin

Creating objects using a primary constructor is the most appropriate approach for the vast majority of objects in our projects. Telescoping constructor patterns should be treated as obsolete in Kotlin. I recom- mend using default values instead, as they are cleaner, more flexible, and more expressive. The classic builder pattern is rarely reasonable. In simpler cases we can just use a primary constructor with named arguments, and when we need to create more complex object we can define a DSL builder for that.

[Thomas Martin]

I have to say the builder DSL looks interesting

[CLOVIS]

The DSL pattern is one of the foundations of Kotlin, hope you'll like it 🙂 there's a lot to say, don't hesitate to ask if you have any questions

TL;DR : scope object + extension lambdas

[Shaun Burch]

my question is why ?

human reviewer told me there are too many params

It comes down to who the users of that code will be. Do they find a large signature which is not using default params excessive to work with. But that’s between you and your team to decide.

[Thomas Martin]

We are having this debate internally, but since we have not all the same opinion I wanted external arguments

[Shaun Burch]

Throw em a curve ball. Tell them it’s going parameterless and will call external variables. Up to them to learn which & where (“wuhaha”).

[Khalil Elias]

Here you have a discussion on the topic, https://wiki.c2.com/?TooManyParameters, not pure Kotlin but OOP.

If we talk about a DDD perspective, I would say that the Primitive Obsession also applies here: https://wiki.c2.com/?PrimitiveObsession, in a sense that you may be sharing multiple correlated domain ideas that might be better represented in a joint object.

From the perspective of impact on maintainability, it depends on who will maintain this in the future. A common side effect is that current/future team members would add a new param for every new required data with little focus on the design.

just my 2 cents