코드 품질과 설계 자문

wayhope Dev Notes

코드 품질과 설계에 대해 자문받고 정리한 내용

이번에 개발과 관련해서 자문을 받으면서 코드 품질에 대한 이야기도 같이 나왔다.

처음에는 그냥 코드를 깔끔하게 작성하는 방법 정도를 생각하고 있었다. 변수 이름을 잘 짓는다거나, 함수를 적당히 나눈다거나 하는 정도다.

그런데 이야기를 듣다 보니 그런 것보다 조금 더 근본적인 부분이 있었다.

코드가 지금 정상적으로 동작하는 것과 나중에 수정하기 좋은 것은 서로 다른 문제라는 것이다.

실제로 개발하다 보면 이런 코드가 있다.

지금은 잘 돌아간다.

그런데 기능 하나를 추가하려고 보면 어디를 건드려야 할지 모르겠다. 한 군데를 수정했는데 다른 곳에서 문제가 생기기도 한다. 비슷한 코드가 여러 군데 있어서 똑같은 수정 작업을 반복하기도 한다.

이런 걸 이야기하면서 나온 개념이 Code Smell이었다.

Code Smell

Code Smell은 버그를 의미하는 게 아니다.

코드가 당장 실행되지 않는 것도 아니다.

그보다는 코드를 보면서 “이 구조는 한번 다시 보는 게 좋겠다”라고 판단하게 만드는 신호에 가깝다.

대표적으로 중복 코드, 너무 큰 클래스, 너무 긴 메서드, 깊은 중첩, 너무 많은 파라미터 같은 것들이 있다.

Duplicated Code
Large Class
God Object
Long Method
Too Many Parameters
Deeply Nested Code
Magic Number
Data Clump
Shotgun Surgery
Feature Envy
Message Chain
Primitive Obsession
Dead Code
Refused Bequest

이걸 전부 외우는 것 자체는 크게 의미가 없다고 생각한다.

실제로 중요한 건 왜 이상한 구조인지 보는 것이다.

코드가 중복되어 있으면 수정할 곳이 늘어난다.

클래스가 너무 커지면 이 클래스가 정확히 무슨 일을 하는지 파악하기 어려워진다.

함수가 너무 길면 한 번에 읽어야 하는 코드가 많아진다.

조건문이 너무 깊으면 실행 조건을 따라가기가 힘들어진다.

결국 코드가 커질수록 사람이 코드를 이해하는 데 드는 비용이 올라간다.

그래서 Code Smell을 발견했다고 바로 “이건 잘못된 코드다”라고 판단하는 것보다는 리팩토링할 필요가 있는지 확인하는 용도로 보는 게 맞다고 이해했다.

그리고 같은 코드라도 프로젝트에 따라 판단이 달라질 수 있다.

작은 프로젝트에서 단순하게 작성한 코드를 굳이 복잡한 구조로 바꾸는 것도 좋은 선택은 아닐 수 있다.

빈 줄에 대한 이야기

개인적으로 재미있었던 부분은 메서드 안의 빈 줄에 대한 이야기였다.

예를 들어 이런 코드가 있다고 해보자.

void Process()
{
    LoadData();

    CalculateData();

    SaveData();
}

보통은 그냥 보기 편하게 빈 줄을 넣었다고 생각한다.

그런데 자세히 보면 LoadData(), CalculateData(), SaveData()가 서로 다른 작업이다.

그러면 이런 생각을 해볼 수 있다.

Process()가 너무 많은 일을 하고 있는 건 아닐까?

빈 줄이 있다고 무조건 문제가 되는 건 아니다.

다만 빈 줄을 기준으로 작업이 명확하게 나뉘어 있다면 그 부분을 메서드나 객체의 책임으로도 나눌 수 있는지 한번 생각해볼 수 있다.

예를 들어 실제 코드는 이런 식으로 바뀔 수도 있다.

void Process()
{
    var data = LoadData();
    var result = Calculate(data);

    Save(result);
}

여기서 중요한 건 빈 줄을 없애는 게 아니다.

빈 줄을 Code Smell이라고 외워버리면 오히려 이상해진다.

빈 줄이 왜 들어갔는지 보는 것이다.

“여기부터 다른 작업인데?”라는 생각이 들었다면 그때 책임 분리를 고민해보는 정도다.

Scope

Scope는 변수나 이름을 어디까지 사용할 수 있는지를 결정한다.

C#에서는 { }에 의해 지역 변수의 범위가 달라진다.

if (condition)
{
    int value = 10;
}

이때 value는 if 블록 밖에서는 사용할 수 없다.

이런 Scope는 당연히 필요한 기능이다.

다만 의미 없이 중괄호를 계속 만들어서 코드를 감싸는 경우도 있다.

이런 경우에는 “왜 이 Scope가 필요한가?”를 한번 생각해볼 필요가 있다.

SonarQube 같은 정적 분석 도구에서도 제어 흐름에 영향을 주지 않는 불필요한 중첩 블록을 문제로 보는 규칙이 있다.

결국 Scope도 문법 자체보다 사용 이유를 봐야 한다.

중첩된 If문

게임 코드를 작성하면 이런 코드가 생각보다 자주 나온다.

if (player != null)
{
    if (player.IsAlive)
    {
        if (player.HasItem)
        {
            if (player.CanUseItem)
            {
                UseItem();
            }
        }
    }
}

이 코드는 틀린 코드는 아니다.

그런데 조건이 하나씩 늘어날수록 읽는 사람이 안쪽까지 계속 들어가야 한다.

조건이 많아졌을 때 Guard Clause를 사용하면 조금 다르게 작성할 수 있다.

if (player == null)
    return;

if (!player.IsAlive)
    return;

if (!player.HasItem)
    return;

if (!player.CanUseItem)
    return;

UseItem();

이렇게 하면 실패하는 조건을 먼저 정리하고 마지막에 실제 작업을 볼 수 있다.

조건 확인
-> 실패하면 종료
-> 다음 조건 확인
-> 실패하면 종료
-> 실제 처리

이런 식의 코드를 Guard Clause라고 한다.

물론 중첩이 있다고 무조건 고쳐야 하는 건 아니다.

foreach (var enemy in enemies)
{
    if (enemy.IsAlive)
    {
        enemy.Update();
    }
}

이 정도는 오히려 자연스럽다.

결국 몇 단계 중첩되어 있느냐만 보는 것보다는 그 안에 얼마나 많은 로직이 들어가 있는지를 같이 봐야 한다.

객체의 내부를 어디까지 봐야 하는가

Tell Don’t Ask라는 개념도 이 부분과 연결해서 이해했다.

예를 들어 이런 코드가 있다고 하자.

if (player.Inventory.Items.Count > 0)
{
    if (player.Inventory.Items[0].IsUsable)
    {
        // ...
    }
}

이 코드는 Player의 Inventory를 보고, Inventory 안의 Items를 보고, 다시 Item의 상태를 확인한다.

외부 코드가 객체 내부 구조를 너무 많이 알고 있다.

상황에 따라서는 다음처럼 만들 수 있다.

if (player.Inventory.HasUsableItem())
{
    // ...
}

또는 아예

player.UseAvailableItem();

처럼 만들 수도 있다.

이런 방식이 항상 정답이라는 이야기는 아니다.

중요한 건 외부에서 객체 내부의 데이터를 꺼내서 판단하는 코드가 너무 많아지고 있지 않은지를 보는 것이다.

객체가 자기 상태를 판단할 수 있는 문제라면 그 책임을 객체 쪽으로 옮기는 것이 더 자연스러울 수도 있다.

Method Chaining

Method Chaining도 자문하면서 같이 이야기했던 부분이다.

예를 들어 LINQ에서는 이런 코드가 흔하다.

query
    .Where(...)
    .OrderBy(...)
    .Select(...)
    .ToList();

이걸 가지고 “체인이 길기 때문에 나쁜 코드다”라고 하면 안 된다.

이런 형태가 오히려 LINQ의 장점을 살리는 경우도 있다.

문제가 될 수 있는 건 객체 내부를 계속 타고 들어가는 형태다.

player
    .GetInventory()
    .GetItems()
    .First()
    .GetWeapon()
    .GetDamage();

이런 코드를 보면 Player가 Inventory를 알고 있어야 하고, Inventory의 Items도 알아야 하고, 그 안의 Weapon까지 알아야 한다.

이런 코드가 여러 곳에 반복된다면 객체 구조가 외부로 너무 많이 노출된 것은 아닌지 확인해볼 필요가 있다.

그래서 Method Chaining 자체가 문제가 아니라 어떤 목적으로 체인을 사용하고 있는지를 봐야 한다.

Closure

Closure는 처음에는 그냥 Lambda와 비슷한 개념으로 생각하기 쉬웠다.

그런데 정확히는 함수가 만들어진 바깥 환경을 계속 참조할 수 있는 특성에 가깝다.

int value = 10;

Func<int> GetValue = () => value;

GetValue가 외부의 value를 참조하고 있다.

JavaScript에서는 이런 식으로 볼 수 있다.

function CreateCounter()
{
    let count = 0;

    return function()
    {
        count++;
        return count;
    };
}

CreateCounter()가 끝난 뒤에도 반환된 함수는 count를 사용할 수 있다.

이런 특성이 Callback이나 Event Handler, Lambda, LINQ 등에서 많이 사용된다.

다만 이것도 편리하다고 무조건 좋은 것은 아니다.

int damage = 100;

Action attack = () =>
{
    enemy.TakeDamage(damage);
};

attack이 damage를 사용하고 있다는 사실이 함수의 인자만 봐서는 보이지 않는다.

코드가 커지면 이런 외부 상태 의존성이 추적하기 어려워질 수 있다.

그래서 Lambda를 사용할 때도 “이 코드가 어떤 외부 변수를 잡고 있는가?”를 생각할 필요가 있다.

SOLID를 다시 보게 된 부분

SOLID는 개발 공부를 하면서 워낙 많이 봤던 내용이다.

그런데 이번에는 각각의 원칙을 외우는 것보다 Code Smell과 같이 보니 조금 다르게 이해됐다.

S -> Single Responsibility Principle
O -> Open/Closed Principle
L -> Liskov Substitution Principle
I -> Interface Segregation Principle
D -> Dependency Inversion Principle

예를 들어 SRP를 단순히 “클래스 하나에는 책임 하나”라고 외우면 애매하다.

실제로는 변경의 이유가 여러 개가 되고 있는지를 보는 편이 이해하기 쉽다.

Player 하나가 이동도 하고 전투도 하고 저장도 하고 네트워크도 처리한다고 해보자.

Player
-> Movement
-> Combat
-> Save
-> Network
-> UI

이 상태에서는 이동 로직을 수정할 때도 Player를 수정하고 저장 로직을 수정할 때도 Player를 수정한다.

이런 구조가 계속 커지면 책임이 섞였다고 볼 수 있다.

OCP도 비슷하다.

예를 들어 무기 종류가 늘어날 때마다 기존 코드에 if를 계속 추가해야 한다면 확장하기 불편해진다.

if (weaponType == Sword)
{
}
else if (weaponType == Bow)
{
}
else if (weaponType == Gun)
{
}

상황에 따라서는 다형성을 사용하는 편이 나을 수 있다.

IWeapon
-> Sword
-> Bow
-> Gun

LSP는 상속 관계에서 부모 타입으로 사용하던 객체를 자식 객체로 바꿨을 때 기존 코드의 의미가 깨지지 않아야 한다는 내용이다.

ISP는 하나의 거대한 인터페이스를 만들어서 필요 없는 기능까지 구현하게 만들지 않는 쪽이다.

DIP는 구체적인 구현에 직접 의존하기보다는 추상화에 의존하도록 만드는 것이다.

이렇게 놓고 보니 SOLID가 각각 따로 존재하는 규칙이라기보다는 코드를 수정하다가 부딪히는 문제를 설명하는 기준에 가깝다는 생각이 들었다.

Dependency Injection

DI도 결국 DIP와 연결된다.

예를 들어:

class Player
{
    private Weapon weapon = new Sword();
}

이렇게 작성하면 Player가 Sword라는 구체적인 구현을 직접 알고 있다.

반면:

class Player
{
    private IWeapon weapon;

    public Player(IWeapon weapon)
    {
        this.weapon = weapon;
    }
}

이런 식으로 만들면 Player가 구체적인 무기를 직접 생성하지 않아도 된다.

Player
-> IWeapon
-> Sword / Bow / Gun

DI를 단순히 생성자를 사용하는 방법이라고 이해하면 부족하다.

객체가 자기 의존성을 직접 만들어내는 대신 외부에서 필요한 것을 전달받도록 만드는 것이 핵심이다.

Unity에서 Singleton을 쓰는 것과 DI를 쓰는 것

Unity에서는 Singleton을 정말 많이 사용한다.

GameManager.Instance
AudioManager.Instance
NetworkManager.Instance

편하다.

그래서 작은 프로젝트에서는 충분히 실용적일 수도 있다.

문제는 프로젝트가 커지면서 모든 코드가 Singleton을 직접 참조하기 시작할 때다.

Player -> GameManager
Enemy -> GameManager
UI -> GameManager
Network -> GameManager

이렇게 되면 누가 무엇에 의존하는지 추적하기 어려워진다.

DI를 사용하면 최소한 의존성이 객체의 구성 과정에서 드러나게 만들 수 있다.

Player -> IGameManager
Enemy -> IGameManager
UI -> IGameManager

그렇다고 Singleton을 무조건 제거해야 한다는 이야기는 아니다.

Singleton이 문제가 되는 상황과 그렇지 않은 상황을 구분해야 한다.

결국 이것도 같은 이야기다.

기술 자체보다 그 기술 때문에 코드가 어떤 상태가 되는지를 보는 것이다.

Zenject

Unity에서 DI를 구성하다 보면 객체가 많아지고 의존 관계도 복잡해질 수 있다.

이런 부분을 관리하기 위해 Zenject 같은 DI Framework를 사용할 수 있다.

기본적으로는 어떤 인터페이스에 어떤 구현을 사용할지 Container에 등록해두고 필요한 객체가 만들어질 때 연결해주는 방식이다.

예를 들면:

Container.Bind<IWeapon>()
    .To<Sword>()
    .AsSingle();

그리고:

public class Player
{
    [Inject]
    private IWeapon weapon;
}

이런 식으로 구성할 수 있다.

Installer
-> Binding
-> Container
-> Object Construction
-> Injection

다만 이것도 프로젝트 크기를 생각해야 한다.

작은 프로젝트에서 DI Framework까지 가져오면 오히려 구조가 복잡해질 수도 있다.

DI가 필요하다고 해서 반드시 Zenject까지 필요한 것은 아니다.

UniRx를 보면서 생각한 것

UniRx는 Unity에서 Reactive Programming을 사용할 수 있도록 해주는 라이브러리다.

기존 Unity 코드에서는 상태를 계속 확인하는 방식이 흔하다.

Update
-> 상태 확인
-> 조건 확인
-> 함수 호출

Reactive Programming에서는 값이 변하는 것을 하나의 흐름으로 보고 그 변화에 반응할 수 있다.

예를 들어:

health
    .Where(x => x <= 0)
    .Subscribe(_ => Die());

이 코드는 체력을 계속 확인하는 대신 특정 조건이 되었을 때 Die()를 호출하는 형태로 표현되어 있다.

Health
-> Observable
-> Where
-> Subscribe
-> Die

이런 구조가 필요한 곳에서는 상당히 깔끔할 수 있다.

반대로 모든 이벤트를 Reactive Stream으로 바꿔야 한다고 생각하면 코드가 더 복잡해질 수도 있다.

그래서 UniRx도 결국 용도를 보고 선택해야 한다.

Subscription과 Dispose

Reactive Programming을 사용할 때는 구독의 생명주기를 같이 봐야 한다.

var subscription = observable.Subscribe(...);

구독을 했는데 더 이상 필요하지 않은 상태에서도 계속 살아 있다면 이벤트가 계속 전달될 수 있다.

Unity에서는 특히 객체의 생명주기와 관련해서 문제가 생길 수 있다.

예를 들어 GameObject가 사라졌는데 Subscription이 계속 남아 있는 경우다.

UniRx에서는 다음처럼 관리할 수 있다.

observable
    .Subscribe(...)
    .AddTo(this);

그래서 Reactive Programming에서는 Observable을 만드는 것만큼 구독이 언제 시작되고 언제 끝나는지도 중요하다.

결국 코드 품질을 어떻게 봐야 하는가

이번 자문을 듣고 나서 가장 크게 바뀐 부분은 여기였다.

예전에는 좋은 코드를 생각할 때 디자인 패턴이나 SOLID 같은 것을 먼저 떠올렸다.

그런데 지금은 조금 다르게 생각한다.

예를 들어 이런 식으로 판단하면 안 된다.

if가 많다 -> 나쁜 코드
Singleton -> 나쁜 코드
Lambda -> 좋은 코드
DI -> 좋은 코드
UniRx -> 좋은 코드
Method Chain -> 나쁜 코드

이렇게 보면 규칙을 지키는 것 자체가 목적이 된다.

실제로 먼저 봐야 하는 건 이런 부분이라고 생각한다.

이 코드는 수정하기 어려운가?
테스트하기 어려운가?
책임이 섞여 있는가?
다른 코드와 너무 강하게 연결되어 있는가?
객체의 내부 구조를 외부에서 너무 많이 알고 있는가?
작은 변경 때문에 여러 곳을 같이 수정해야 하는가?
객체의 생명주기를 추적하기 어려운가?
불필요한 비용을 만들고 있는가?

문제가 확인되면 그때 방법을 선택한다.

Guard Clause가 필요할 수도 있고, 메서드를 분리할 수도 있다.

인터페이스가 필요할 수도 있고 DI가 필요할 수도 있다.

어떤 경우에는 그냥 지금 구조를 유지하는 것이 더 나을 수도 있다.

이게 중요한 부분인 것 같다.

자문을 받고 나서 정리한 것

결국 Code Smell, SOLID, DI, Closure, Reactive Programming 같은 내용을 따로 외우는 것보다 코드를 보는 기준을 만드는 게 더 중요하다고 생각한다.

이 코드의 책임은 무엇인가?
누가 이 코드를 변경하게 되는가?
무엇에 의존하고 있는가?
변경했을 때 어디까지 영향이 가는가?
객체의 내부를 외부에서 너무 많이 알고 있지는 않은가?

이런 질문을 하다 보면 Code Smell이 보이고, 그다음에 SOLID나 캡슐화 같은 개념을 적용할 이유가 생긴다.

DI도 마찬가지고 Zenject도 마찬가지다.

필요해서 사용하는 것이지, 좋은 기술이니까 넣는 것은 아니다.

개발하면서 결국 계속 마주치는 문제는 비슷하다.

코드가 커졌을 때 수정하기 어려워지고, 책임이 섞이고, 의존성이 복잡해지고, 변경의 영향 범위가 커진다.

좋은 설계라는 것도 결국 이런 문제를 줄이는 방향으로 가는 것이라고 이해했다.

명확한 책임
-> 필요한 만큼의 추상화
-> 적절한 의존성
-> 낮은 결합도
-> 높은 응집도
-> 변경하기 쉬운 코드

이번 자문에서 가장 기억에 남은 건 특정 패턴 하나가 아니라 코드에 문제가 보였을 때 무조건 규칙을 적용하는 게 아니라, 왜 문제가 되는지를 먼저 생각해야 한다는 것이었다.

결국 설계는 정답을 맞히는 문제가 아니라 현재 프로젝트에서 발생하는 변경과 복잡성을 얼마나 적은 비용으로 다룰 수 있느냐의 문제에 가깝다고 생각한다.