```listOf( this.session.classFromId(ClassId.to...

# getting-started

[min]

listOf(
    this.session.classFromId(ClassId.topLevel(FqName(/* path */))) ?: continue,
    this.session.classFromId(myAnnotation.annotationArgument) ?: continue,
)

What’d be the best way to make this code more DRY?

[min]

listOf(
    ClassId.topLevel(FqName(/* path */)),
    myAnnotation.annotationArgument,
)
    .map { id -> this.session.classFromId(id) ?: continue }

Is this the best I can do?

[Joffrey]

It might be worth sharing more code around it, since there is a continue here

[Joffrey]

(but otherwise your new option seems like a decent step)

[min]

do {
    if (callableId.callableName != COMPANION_METHOD_TO_GENERATE) { continue }
    companionObject = context?.owner ?: continue

    val myAnnotation = this.session.enclosingClass(companionObject)
        ?.let(this.session::parseMyAnnotation)
        ?.asFoo() ?: continue
    companionMethodAnnotations = listOf(
        this.session.classFromId(ClassId.topLevel(FqName(/* path */))) ?: continue,
        this.session.classFromId(myAnnotation.annotationArgument) ?: continue,
    )
    annotatedTypeParams = myAnnotation.annotatedClass.typeParameterSymbols
    annotatedContextParams = myAnnotation.primaryConstructor.contextParameterSymbols
    annotatedValueParams = myAnnotation.primaryConstructor.valueParameterSymbols
    annotatedReturnType = myAnnotation.primaryConstructor.resolvedReturnType
    break
} while (return emptyList())

[Marat Akhin]

My humble opinion is that if following DRY destroys structured control flow, you should not try to follow DRY.

[min]

I don’t know what you mean

[Marat Akhin]

continue and break have a very specific purpose and meaning in control flow, abusing them to do what you did, in my opinion, hurts readability and code comprehension much more that potentially not following DRY.

[min]

If I had used Arrow, the same code would have looked something like this:

nullable {
    if (callableId.callableName != COMPANION_METHOD_TO_GENERATE) { bind() }
    companionObject = context?.owner.bind()

    val myAnnotation = this.session.enclosingClass(companionObject)
        ?.let(this.session::parseMyAnnotation)
        ?.asFoo().bind()
    companionMethodAnnotations = listOf(
        this.session.classFromId(ClassId.topLevel(FqName(/* path */))).bind(),
        this.session.classFromId(myAnnotation.annotationArgument).bind(),
    )
    annotatedTypeParams = myAnnotation.annotatedClass.typeParameterSymbols
    annotatedContextParams = myAnnotation.primaryConstructor.contextParameterSymbols
    annotatedValueParams = myAnnotation.primaryConstructor.valueParameterSymbols
    annotatedReturnType = myAnnotation.primaryConstructor.resolvedReturnType
} ?: return emptyList()

It’s 95% the same, and the different 5% comprises trivial things as replacing do with nullable and ?: continue with .bind(). This Arrow code is absolutely fine, but I didn’t feel like bringing in a whole dependency for the 5% of things. I’ll just write a comment instead.

[Marat Akhin]

I feel you're completely missing the point, but -- in the end, it's your code and you can do whatever you want with it 😃

[min]

Yes, I don’t think this ‘destroys structured control flow’ and ‘hurts readability’.

if (callableId.callableName != COMPANION_METHOD_TO_GENERATE) { return emptyList() }
companionObject = context?.owner ?: return emptyList()

val myAnnotation = this.session.enclosingClass(companionObject)
    ?.let(this.session::parseMyAnnotation)
    ?.asFoo() ?: return emptyList()
companionMethodAnnotations = listOf(
    this.session.classFromId(ClassId.topLevel(FqName(/* path */))) ?: return emptyList(),
    this.session.classFromId(myAnnotation.annotationArgument) ?: return emptyList(),
)
annotatedTypeParams = myAnnotation.annotatedClass.typeParameterSymbols
annotatedContextParams = myAnnotation.primaryConstructor.contextParameterSymbols
annotatedValueParams = myAnnotation.primaryConstructor.valueParameterSymbols
annotatedReturnType = myAnnotation.primaryConstructor.resolvedReturnType

This is an alternative example that ‘doesn’t follow DRY’, and I have to duplicate (the early return) logic several times. If I were to change how null is handled from return emptyList() to a different strategy, I need to find and update every copy.

do {
    // logic that might exit early (`continue`)
    break // happy path
} while (/* handle early exits here */)

It’s better to learn this as a recipe once and use it everywhere. What it looks like to take DRY too far in my book is something like this:

buildSomething {
    x = f()
    y = g()
}

I’d eventually like a linter that flags all code like this. Instead, I always write:

buildSomething(fun (builder) {
    builder.x = f()
    builder.y = g()
})

My recipe lets you avoid duplicating logic. buildSomething just lets you avoid duplicating identifiers. It‘s fine to duplicate identifiers, because I declared builder to use it many times in the first place. I plan on eventually authoring a plugin to make the recipe even cleaner! Also I don’t know why we’re discussing DRY in general, my question was about the following list specifically.

listOf(
    this.session.classFromId(ClassId.topLevel(FqName(/* path */))) ?: continue,
    this.session.classFromId(myAnnotation.annotationArgument) ?: continue,
)

[Marat Akhin]

Within your example you're optimizing for code writing and not code reading.

"If I were to change how null is handled from return emptyList() to a different strategy, I need to find and update every copy."

This is something you are doing less often than reading your code.

Also let's agree to disagree on whether "I'm giving continue a completely different meaning within this code" does or does not destroy traditional structured control flow and how easy it is to read and reason about.

Also I don’t know why we’re discussing DRY in general

Because your initial question was "What’d be the best way to make this code more DRY?" And we are discussing whether to follow DRY w.r.t. the snippet of break while (return emptyList()) code you've shared.

[min]

The `do`–`while` version and nullable version are about as readable as each other. In order to understand either, one needs to learn:
1. The role of the do block (enable the use of continue), or about the nullable function,
2. What continue achieves (an early exit handled by the while), or about the bind method.
Once one has learnt the recipe or about the nullable and bind methods, the rest is literally identical. It doesn’t take meaningfully more or less to understand one version than the other. So I assume you have the same complaints about nullable + bind, which I don’t. To each their own!

Because your initial question was "What’d be the best way to make this code more DRY?"

Yes, said ‘this code’ being the list.

[Marat Akhin]

So I assume you have the same complaints about nullable + bind, which I don’t.

I don't have problems with that version, and the reason is simple -- that version uses completely separate vocabulary which immediately tells the reader "this does bind thingy, go learn what it is". Your version tells the reader "this does continue thingy", and then also goes and redefines the pretty fundamental meaning of continue within Kotlin. And that increases cognitive load on the reader.

[min]

Ah, the good news is once I’ve worked on the plugin, the underlying do block will be wrapped and hidden!

[Marat Akhin]

I don't have problems with that version.

Correction: my preferred version is the one which just does return emptyList() when it needs to express exactly that.

[min]

I don’t duplicate logic.

[phldavies]

why not use something like:

run {
  val blah ?: return@run // early escape uses `return@run` which is clearer than `continue` 
  // ...
  return listOf(realValue);  // happy return or further logic at the end of the lamdba
}
return emptyList() // sad return if run was interrupted

[phldavies]

I'd advise against run<Unit?> { ... } ?: return emptyList() as you'd need to ensure the last statement of your "happy" path through the run doesn't result in a null return.

[phldavies]

run {
    // logic that might exit early (`return@run`)
    return // happy path
}
/* handle early exits here */)

[min]

return emptyList() is an implementation detail. I Exit Early in several different places, however that happens to be implemented at the moment. I need a way to invoke that without duplicating and hardcoding the current implementation everywhere I need it.

override fun generateFunctions(
    callableId: CallableId,
    context: MemberGenerationContext?,
): List<FirNamedFunctionSymbol> {
    val companionObject: FirClassSymbol<*>
    val companionMethodAnnotations: List</* TODO */>
    val annotatedTypeParams: List<FirTypeParameterSymbol>
    val annotatedContextParams: List<FirValueParameterSymbol>
    val annotatedValueParams: List<FirValueParameterSymbol>
    val annotatedReturnType: ConeKotlinType
    do {
        if (callableId.callableName != COMPANION_METHOD_TO_GENERATE) { continue }
        companionObject = context?.owner ?: continue

        val myAnnotation = this.session.enclosingClass(companionObject)
            ?.let(this.session::parseMyAnnotation) // : Either<MyAnnotation, *>
            ?.asFoo() ?: continue
        companionMethodAnnotations = listOf(
            ClassId.topLevel(FqName(/* hardcoded path */)),
            myAnnotation.annotationArgument,
        )
            .map(/* Transform IDs to annotations */)
        annotatedTypeParams = myAnnotation.annotatedClass.typeParameterSymbols
        annotatedContextParams = myAnnotation.primaryConstructor.contextParameterSymbols
        annotatedValueParams = myAnnotation.primaryConstructor.valueParameterSymbols
        annotatedReturnType = myAnnotation.primaryConstructor.resolvedReturnType
        break
    } while (return emptyList())

    return listOf(SubstitutedMethodSignatureBuilder.new(
        session = this.session,
        callableId = callableId,
        enclosingClass = companionObject,
        sourceTypeParams = annotatedTypeParams,
        key = KEY,
    )
        .addContextParamSubstitutes(annotatedContextParams)
        .addValueParamSubstitutes(annotatedValueParams)
        .addAnnotations(companionMethodAnnotations)
        .build(
            annotatedReturnType.toFirResolvedTypeRef(),
            substitute = true,
        )
        .symbol
    )
}

I used `do`–`break`–`while` instead of run because the happy path continues after the code with fallible logic. I like to keep the happy path dedented as much as possible.