Post

라인의 코드 품질 개선 기법 리포트를 읽고

닉네임 도입에 따른 loginName/displayName 네이밍 정리, 컬렉션 노출의 스냅샷/불변 처리, enum ↔ 외부값 양방향 변환의 단일 소스 유지, 레포지토리–UI 경계 분리를 위한 일급 모델(hasMoreItems) 도입 등 4가지 개선 사례를 코드와 함께 정리합니다.

Preview Image
15
라인의 코드 품질 개선 기법 리포트를 읽고
라인, 클린코드
클린코드

개선 7편 : 새것을 들일 때 옛것도 다시 살피자

사용자라는 모델 클래스가 있다.

1
2
3
4
5
6

class UserData(
    val id: UserId,
    val name: String,
    val profileImageUri: String,
    ...
)

name 은 사용자의 이름을 나타낸다. -> 이때, 로그인 이름을 다른 사용자에게 공개하고 싶지 않다는 요구 사항을 구현하기 위해 ‘닉네임’ 을 등록하도록 사양을 변경한다고 하자.

1
2
3
4
5
6
7
8
9
10
11

class UserData(
    val id: UserId,
    val name: String,
    val profileImageUri: String,
    ...
    /**
     * ...
     * 만약 `nickname`이 명시적으로 `null`인 경우 닉네임이 등록되지 않았음을 의미한다.
     */
    val nickname: String?
)

닉네임이 등록하지 않으면 null 을 사용하는 건 합의된 결과

1
2

userNameView.text = userData.nickname
    ?: userData.name

이와같이 사용하면 뭐가 문제가 될까.

새로운 것을 추가할 때 오래된 것을 바꾸기

1
2
3

val nickname = userData.name
nicknameView.isVisible = nickname.isNotEmpty()
nicknameView.text = nickname

닉네임이 아닌 로그인 이름을 그대로 사용했다는 걸, 알기 어렵게 된다. ( nickname 이 name 의 일종처럼 보이기 때문 )

UserData.nickname 을 추가할 때, UserData.name 의 이름을 변경하는게 좋다.

1
2
3
4
5
6
7
8
9

class UserData(
    val id: UserId,
    val loginName: String,
    val nickname: String?,
) {
    // Also, we may put property selection logic
    val displayName: String
        get() = nickName ?: loginName
}

두 개의 차이를 명확하게 구분 할 수 있게 된다.

사례

1
2
3
4
5
6
7

기존 요소 : 생성 시점 타임 스탬프 - timestamp
새로운 요소 : 마지막 업데이트 타임 스탬프 - lastUpdateTimeStamp
=> timestamp -> creationTimeStamp

기존 요소 : 사용자 프로필에 추가할 수 있는 외부 링크 : url
새로운 요소 : 사용자 프로필 이미지 링크 : profileImageUrl
=> url -> externalLinkUrl

근데, 이러면 DB 에 저장된 칼럼의 이름은 어떻게 할 것인가… 어마어마한 데이터가 있으면 그냥 적절히 어노테이션으로 이름만 바꾸는게 최선일 것

개선 8편 : 실상과 허상

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

class UserNameMapLoader(private val apiClient: ApiClient) {
    private val userNameMap: MutableMap<UserId, String> = mutableMapOf()
    private var lastChunkIndex: Int = -1

    val loadedUserNameMap: Map<UserId, String>
        get() = userNameMap

    val loadedUserIds: Set<UserId>
        get() = userNameMap.keys

    val decoratedUserNameMap: Map<UserId, String>
        get() = userNameMap.mapValues { (_, value) -> "**$value**" }

    ...

    fun loadNextChunk() {
        val response = apiClient.requestChunk(lastChunkIndex + 1) as? Success
            ?: return

        userNameMap += response.chunk.toMapEntries()
        lastChunkIndex++
    }
}

apiClient 를 통해 사용자의 ID 와 이름을 관리한다.

  • 조회하려는 데이터는 한 번에 모두 가져올 수 없고, 청크를 구분해서 가져온다
  • 해당 코드는 특정 스레드에서 실행, 조회할 수 있는 데이터는 변하지 않고 정렬되어 있다고 가정

돌은 제멋대로 굴러간다

1
2
3
4
5
6
7
8
9

val userNameMap = loader.loadedUserNameMap
val userIds = loader.loadedUserIds
val decoratedUserNameMap = loader.decoratedUserNameMap

loadNextChunk()

println(userNameMap) // Shows {3=Alice, 5=Bob, 7=Charlie, 11=Dave}
println(userIds) // Shows [3, 5, 7, 11]
println(decoratedUserNameMap) // Shows {3=**Alice**, 5=**Bob**}

위 3개는 유사해보이지만, 무언가의 차이가 있다.

1
2
3
4
5

val loadedUserNameMap: Map<UserId, String>
	get() = userNameMap

val loadedUserIds: Set<UserId>
	get() = userNameMap.keys

두 개는 원본 Map 에 대한 참조이므로, 외부 loader 에 의해 변경될 수 있다.

1
2

val decoratedUserNameMap: Map<UserId, String>
	get() = userNameMap.mapValues { (_, value) -> "**$value**" }

속성에 접근할 때, Map 을 복사해서 반환한다. -> loader 에 의해 변경되지 않는다.

이를 해결하기 위한 옵션이 몇가지 있다.

옵션 1 : 요소에 대한 접근 허용

1

fun getName(userId: UserId): String? = userNameMap[userId]

Map, Set 을 반환하지 않으면 이후 업데이트는 걱정할 필요도 없게 된다.

하지만, UserId 를 별도로 관리하고, 청크로 조회할 때도 이 ID 목록을 넘겨줘야만 한다.

사실상 불가능하다..

옵션 2 : 복사본만 반환

1
2

fun createUserNameMapSnapshot(): Map<UserId, String> =
	userNameMap.toMap() // `toMap` is to create a copy of the map.

함수 이름에 복사본을 반환한다는 것을 명확하게 하자. + toList, toMap 에도 주석을 달면 실수로 복사본 생성하는 코드를 삭제하는 것도 방지 가능하다

이 역시도 성능에 영향을 줄 수 있다. Copy-On-Write 메커니즘이 아니라면 성능에 영향을 준다.

COW : 쓰기를 하는 순간 복사, 초기에는 단순 참조만 복사 - 메모리는 효율적, 첫 쓰기 성능은 느려짐

옵션 3 : 직접 참조만 반환

1
2
3
4
5
6
7

 /**
 * A map with UserId as the key and usernames.
 *
 * This map is read-only, but the contents can be changed through
 * operations on this `UserNameMapLoader`.
 */
val userNameMap: Map<UserId, String> = mutableUserNameMap

참조를 그대로 반환해야 한다면, 최악을 피하기 위해 주의사항을 문서에 언급을 해야만 한다.

개선 9편 : 왔던 길을 되돌아가 보자

네트워크, 파일 시스템 같은 I/O 를 사용하면, I/O 데이터 표현과 코드의 데이터 표현을 상호 변환해야만 한다.

EX) 인터페이스 정의 언어, DB 스키마 등 외부에서 정의된 데이터와 코드에서 정의된 모델 클래스를 상화 변환

이때, ‘상태’, ‘타입’ 등을 나타내는 값을 사용한다면, ENUM 을 사용하기도 한다.

1
2
3
4
5
6
7
8
9
10
11
12
13

enum class AccountType { FREE, PREMIUM, ULTIMATE }

val DB_VALUE_TO_ACCOUNT_TYPE_MAP: Map<Int, AccountType> = mapOf( // or SparseArray
    0 to AccountType.FREE,
    1 to AccountType.PREMIUM,
    2 to AccountType.ULTIMATE
)

val ACCOUNT_TYPE_TO_DB_VALUE_MAP: Map<AccountType, Int> = mapOf( // or EnumMap
    AccountType.FREE to 0,
    AccountType.PREMIUM to 1,
    AccountType.ULTIMATE to 2
)

꽤나 신기한 코드군, 자바로선..

평행한 두 개의 도로

열거형으로 변환하는 것과, 반대로 변환하는 것이 별도 정의되어 있기 때문에 두 가지 문제가 발생한다.

  • 사양 변경 시 두 Map 은 모두 업데이트 되어야 한다
  • 두 변환 간에 서로 대응이 잘 이뤄지는지 보장할 수 없다

이를 위해 when / switch 를 사용해, Map 대신 사용해 보장하기 쉽다.

구현 예제 1: 열거자 속성 & 역변환 맵

1
2
3
4
5
6
7
8
9
10

enum class AccountType(val dbValue: Int) {
    FREE(0),
    PREMIUM(1),
    ULTIMATE(2);
    
    companion object {
        val DB_VALUE_TO_TYPE_MAP: Map<Int, AccountType> =
            AccountType.entries.associateBy(AccountType::dbValue)
    }
}

1
2

accountType.dbValue :  // AccountType -> DB
AccountType.DB_VALUE_TO_TYPE_MAP[dbValue] // DB -> AccountType

이때, 여러 데이터 표현 간 상호 변환이 필요하면 dbValue 가 여러개 정의될 수 있다.

1
2
3
4
5
6
7

enum class AccountType(
    val dbValue: Int,
    val fooApiJsonValue: String,
    val barApiJsonValue: String
) {
    ...
}

지옥이 시작된다…

구현 예제 2 : 개별 레이어 내에서 변환 구현 및 역변환 맵

serverValue 와 AccountType 의 상호변환을 AccountTypeNetworkClient 에 국한시킨다.

1
2
3
4
5
6
7
8
9
10
11
12
13

class AccountTypeNetworkClient {

    companion object {    
        private fun AccountType.toServerValue(): String = when (this) {
            AccountType.FREE -> "free"
            AccountType.PREMIUM -> "premium"
            AccountType.ULTIMATE -> "ultimate"
        }

        private val SERVER_VALUE_TO_TYPE_MAP: Map<String, AccountType> =
            AccountType.entries.associateBy { it.toServerValue() }
    }
}
  • when 을 통해 모든 열거자를 포괄하는 것을 보장

또는, Mapper & Converter 같은 클래스를 별도로 정의하는 것도 하나의 방법 ( 이때도, 당연히 순방향 & 역방향 변환은 같이 위치시킬 것 )

1

assertEquals(DB_VALUE_TO_TYPE_MAP.size, AccountType.entries.size)

단사(값의 중복이 없음)를 보장하는 테스트 역시도 만들어둘 것

이거 되게 중요한게, 이렇게 간단한 테스트를 왜 만들어 둔 거지.? 라고 생각할 수 있는데 이런 단순한 테스트가 쌓여서 코드를 지탱한다.

=> 양방향 변환을 할 때는 한쪽 변환 로직에서 다른 쪽 로직을 연역적으로 구하는 것이 바람직하다.

개선 10편 : 적절한 거리 유지에 신경 쓰자

1
2
3
4
5
6
7

Items: 3
---------
 <item1>
---------
 <item2>
---------
 <item3>

Item 의 총 개수를 표시하는 헤더가 있다고 할 때

  • 목록에 표시할 수 있는 Item 개수는 처음 100개 까지
  • 100개를 초과하는 Item 이 있는 경우 100+ 로 표시
1
2
3
4
5
6
7
8
9

Items: 100+
---------
 <item1>
---------
 <item2>
---------
... (생략) ...
---------
 <item100>

이를 위해

1
2
3
4
5

// Model classes
const val ITEM_LIST_MAX_COUNT = 100

class Item(...)
class StoredItems(val items: List<Item>)

1
2
3
4
5
6
7
8

// In the repository layer
class ItemRepository(...) {
    suspend fun getItemList(): StoredItems {
        // `+ 1` is for showing "+" on the UI.
        val items = itemDao.selectItems(ITEM_LIST_MAX_COUNT + 1)
        return StoredItems(items)
    }
}

이렇게 Model, Repository 를 정의했다.

1
2
3
4
5
6

val itemCount = storedItems.items.size
countTextView.text =
    if (itemCount <= ITEM_LIST_MAX_COUNT) itemCount.toString() 
    else "$ITEM_LIST_MAX_COUNT+"

itemListAdapter.items = storedItems.items.take(ITEM_LIST_MAX_COUNT)

UI 에선 Item 개수가 최대 개수를 초과하는 지 아닌지 따라 분기된다.

‘+1’ 만큼의 거리를 유지하자

코드를 여러 레이어나 컴포넌트로 나눌 때는 어떤 레이어, 컴포넌트가 어떤 정보를 가져야 하는지 를 고려해서 설계해야 한다.

하지만, 위 코드는 지금 UI - 레포지토리 레이어 간 ‘암묵적인 정보’ 공유하므로 사양 변경시 버그를 발생하기 쉽다.

  • // + 1 is for showing "+" on the UI. 라는 주석은 UI 의 세부 사항을 알려주고 있다.

  • UI 측도 레포지토리 레이어의 세부 사항에 의존하고 있다. UI 측에 countTextView.text 를 구하는 로직이 ITEM_LIST_MAX_COUNT 보다 큰 목록을 반환한다 는 동작에 의존하고 있다.

이 문제를 위해 일급 객체를 만들어 해결할 수 있다.

1

class StoredItems(val items: List<Item>, val hasMoreItems: Boolean)

레포지토리 레이어는 UI 세부 사항을 신경 쓰지 않고, 모델 클래스를 통해 인스턴스 생성에 집중할 수 있다.

1
2
3
4
5
6
7
8
9
10
11
12

private const val ITEM_LIST_MAX_COUNT = 100

class ItemRepository(...) {
    suspend fun getItemList(): StoredItems {
        // `+ 1` is for deciding `hasMoreItems`.
        val items = itemDao.selectItems(ITEM_LIST_MAX_COUNT + 1)
        return StoredItems(
            items.take(ITEM_LIST_MAX_COUNT),
            items.size > ITEM_LIST_MAX_COUNT
        )
    }
}

UI 레이어 역시도, ITEM_LIST_MAX_COUNT 에 대한 정보가 없어도 된다.

1
2
3
4

val countText = storedItems.items.size.toString()
countTextView.text = if (storedItems.hasMoreItems) "$countText+" else countText

itemListAdapter.items = storedItems.items

( 단순히, hasMoreItems 로 판별 )

그러면, ITEM_LIST_MAX_COUNT 가 레포지토리 레이어에 있는건 괜찮은가

비즈니스 로직 레이어에 마련하고, 그곳에 정의해도 된다. ( 아키텍처 따라, 도메인 & 서비스 & 유스 케이스 등등 )

또는, 모델 클래스에 해당 정보를 넣는것도 하나의 방법

왜 우리가 일급 객체를 써야 하는지. 에 대해 다시 한번 리마인드 하자

하지만, 알고리즘 -> 데이터 구조 라는 의존 관계 방향이 모호해질 수 있다.