CODEWITHSUNDEEP
javaif-statementkotlinsimplify
J. Adam
J. Adam

Reputation: 1641

How to make this complex if-else statement maintainable in Kotlin

I wrote a function to get information out of the database based on requestparams. The following if-else statement is a huge problem. If we keep adding more filters we need to keep adding statement for all the possible paths.

    fun getMessages(name: String, pageable: Pageable, locale: String?, subject: String?,
                              recipient: String?): Page<MessageDTO>? {

        val messagePageable= if (!locale.isNullOrEmpty() && !subject.isNullOrEmpty() && !recipient.isNullOrEmpty()) {
            messageRepository.findAll(where(hasMessageName(name).and(hasLocale(locale!!)
            .and(hasSubject(subject!!).and(hasRecipient(recipient!!))))), pageable)
        } else if (!locale.isNullOrEmpty()) {
            messageRepository.findAll(where(hasMessageName(name).and(hasLocale(locale!!))), pageable)
        } else if (!subject.isNullOrEmpty()) {
            messageRepository.findAll(where(hasMessageName(name).and(hasSubject(subject!!))), pageable)
        } else {
            messageRepository.findAll(where(hasMessageName(name)), pageable)
        }
        return messagePageable.map { messageMapper.toMessageDTO(it) }.takeIf { it.content.isNotEmpty() }
}

There should be a better way of writting this. I appreciate your help.

Upvotes: 2

Views: 642

Answers (2)

Demigod
Demigod

Reputation: 5635

I'm not sure, but maybe some refactoring will help. For example it seems like you're using some sort of request to the db, that if filled according to the parameters, so maybe it can be managed like this.

fun getMessages(name: String, pageable: Pageable, locale: String?, subject: String?,
                          recipient: String?): Page<MessageDTO>? {
    val request = where(hasMessageName(name))
    locale?.let{ request.and(hasLocale(it)) }
    subject?.let{ request.and(hasSubject(it)) }
    recipient?.let{ request.and(hasRecipient(it))}

    return messageRepository.findAll(request, pageable)
                            .map { messageMapper.toMessageDTO(it) }
                            .takeIf { it.content.isNotEmpty() }
}

Here, I'm not sure how .and() method works, so maybe it should be like: request = request.and(...)

Upvotes: 5

msrd0
msrd0

Reputation: 8361

You can replace your if statement with a when statement to make a bit easier to read:

fun getMessages(name: String, pageable: Pageable, locale: String?, subject: String?,
                              recipient: String?): Page<MessageDTO>? = when {
    !locale.isNullOrEmpty() && !subject.isNullOrEmpty() && !recipient.isNullOrEmpty() ->
        messageRepository.findAll(where(hasMessageName(name).and(hasLocale(locale!!)
            .and(hasSubject(subject!!).and(hasRecipient(recipient!!))))), pageable)
    !locale.isNullOrEmpty() ->
        messageRepository.findAll(where(hasMessageName(name).and(hasLocale(locale!!))), pageable)
    !subject.isNullOrEmpty() ->
        messageRepository.findAll(where(hasMessageName(name).and(hasSubject(subject!!))), pageable)
    else ->
        messageRepository.findAll(where(hasMessageName(name)), pageable)
}.map { messageMapper.toMessageDTO(it) }.takeIf { it.content.isNotEmpty() }

Upvotes: 0

Related Questions