거대한 소프트웨어를 유지보수하는 것은 중요하다. 그래서 우리는 리팩토링 기법등을 배우며 오래된 코드베이스를 좀 더 읽기 좋고 변화하기 쉽도록 다시 고쳐쓴다. Django 코드를 예시로 들어 살펴보자.
복잡한 함수 단순화하기
다음과 같은 함수에서 출발해보자.
from django.db import models
from django.utils import timezone
class User(models.Model):
should_verify = models.BooleanField(default=True)
last_verified_at = models.DateTimeField(auto_now_add=True)
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if self.should_verify:
if self.last_verified_at:
time_diff = timezone.now() - self.last_verified_at
if time_diff.days < 180:
return True
else:
return False
return False
return True어떤가, 눈에 잘 들어오는가? 그렇지 않다. 왜 그럴까?
1. if문이 중첩되어있고
2. 주석에 써 있는 생각의 흐름대로 코드가 진행되지 않기 때문이다.
Early Return 적용하기
우선 가장 바깥쪽의 if문을 early return으로 변경해보자. 다음과 같은 패턴을 적용하면 된다. Refactoring(Martin Folwer): Replace Nested Conditional with Guard Clauses
class User(models.Model):
# ...
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at:
time_diff = timezone.now() - self.last_verified_at
if time_diff.days < 180:
return True
else:
return False
return False어떤가? 좀 더 나아졌다. 인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.에서 인증할 필요가 없거나를 처리한 것이다. 한번 더 적용할 수 있을 거 같다. 다시 해보자.
class User(models.Model):
# ...
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
time_diff = timezone.now() - self.last_verified_at
if time_diff.days < 180:
return True
else:
return False숨어있는 함수 추출하기
이제 위 과정을 거쳤으니, 나머지 논리가 드러났다. time_diff = timezone.now() - self.last_verified_at를 구해 180일이 지났는지 확인하는 것은 너무 세부 구현에 가까운 것 같다. 함수로 추출해보자.
class User(models.Model):
# ...
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
if self.elapsed_after_verified.days < 180:
return True
else:
return False
@property
def elapsed_after_verified(self):
return timezone.now() - self.last_verified_at위와 같이 elapsed_after_verified라는 좀 더 작은 단위를 뽑아냈다. 그런데 이 함수를 굳이 외부로 노출할 필요가 있을까? 그러지 말자.
class User(models.Model):
# ...
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
if self._elapsed_after_verified.days < 180:
return True
else:
return False
@property
def _elapsed_after_verified(self):
return timezone.now() - self.last_verified_at좋다. 이제 protected된 함수가 생겼다. 이제 조건절을 하나 제거할 수 있을 거 같다.
class User(models.Model):
# ...
def verified(self) -> bool:
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
return self._elapsed_after_verified.days < 180
# ...됐다. 이제 불필요한 if문을 하나 없앴다.
상수 추출하기
그런데, 180이라는 것이 뭐를 의미하나? 어떤 의미를 갖는 Magic Number는 이름을 붙여줘야 한다. 다음과 같이 해보자.
class User(models.Model):
# ...
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
verification_valid_days = 180
return self._elapsed_after_verified.days < verification_valid_days
굳이 메소드 내에서 매번 재할당을 할 필요가 없을 것 같다. 상수로 빼 보자.
from django.db import models
from django.utils import timezone
VERIFICATION_VALID_DAYS = 180
class User(models.Model):
should_verify = models.BooleanField(default=True)
last_verified_at = models.DateTimeField(auto_now_add=True)
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
return self._elapsed_after_verified.days < VERIFICATION_VALID_DAYS
@property
def _elapsed_after_verified(self):
return timezone.now() - self.last_verified_at이제 함수 내부에서 고칠것은 없어 보인다. 하지만 이름이 좀 거슬린다.
Convention에 맞는 이름 쓰기
다음을 생각해보자.
# 1번
if user.verified():
# ...
# 2번
if user.is_verified:
# ...어느 쪽이 좀 더 자연스러운가? 2번이 훨씬 자연스럽다. Ruby에서 boolean flag뒤에 ?를 붙이듯 파이썬의 경우 boolean 메소드 / 변수 앞에 is_를 붙이면 훨씬 자연스럽다. 이제 그렇게 이름을 바꾸고 property로 만들어주자.
from typing import Final
from django.db import models
from django.utils import timezone
VERIFICATION_VALID_DAYS: Final[int] = 180
class User(models.Model):
should_verify = models.BooleanField(default=True)
last_verified_at = models.DateTimeField(auto_now_add=True)
@property
def is_verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if not self.should_verify:
return True
if self.last_verified_at is None:
return False
return self._elapsed_after_verified.days < VERIFICATION_VALID_DAYS
@property
def _elapsed_after_verified(self) -> "timedelta":
return timezone.now() - self.last_verified_at처음과 비교해보자.
from django.db import models
from django.utils import timezone
class User(models.Model):
should_verify = models.BooleanField(default=True)
last_verified_at = models.DateTimeField(auto_now_add=True)
def verified(self) -> bool:
"""
인증할 필요가 없거나 인증후 180일이 지나지 않았어야 한다.
"""
if self.should_verify:
if self.last_verified_at:
time_diff = timezone.now() - self.last_verified_at
if time_diff.days < 180:
return True
else:
return False
return False
return True이제 읽기 힘들게 깊이 중첩된 if문도, 마법같은 상수도 없으며 외부로 제공하는 인터페이스도 깔끔해졌다.
내부 구현 숨기기
우리가 출발할 코드는 다음과 같다. 그렇게 나쁘지는 않다. 하지만 좀 더 좋아질 구석들이 존재한다.
# models/user_block.py
from django.db import models
from django.db.models import UniqueConstraint
class UserBlock(models.Model):
user = models.ForeignKey("User", related_name="blocks", on_delete=models.CASCADE)
issued_date = models.DateTimeField(auto_now_add=True, null=True)
blocked_user = models.ForeignKey(
"User", related_name="blocked_blocks", on_delete=models.CASCADE
)
class Meta:
constraints = [
UniqueConstraint(fields=("user", "blocked_user"), name="unique_user_block")
]
# models/user.py
from django.db import models
from django.db.models import F
from django.core.cache import cache
class User(models.Model):
nickname = models.CharField(max_length=25, unique=True)
user_blocks = models.ManyToManyField(
"user.User",
through="UserBlock",
blank=True,
through_fields=("user", "blocked_user"),
)
class Meta:
verbose_name = "유저"
def did_block(self, user_id):
user_blocks = self.get_user_blocks()
return any(user_block["user_id"] == user_id for user_block in user_blocks)
def get_user_blocks(self):
data = cache.get(f"{self}:user-blocks")
if data is not None:
return data
blocks = self.user_blocks.values(
"nickname",
user_id=F("id"),
issued_date=F("blocked_blocks__issued_date"),
)
cache.set(f"{self}:user-blocks", blocks)
return blocksGetter는 Property로 && 의도를 드러내는 이름
# models/user.py
class User(models.Model):
# ...
def did_block(self, user_id):
user_blocks = self.blocked_users
return any(user_block["user_id"] == user_id for user_block in user_blocks)
@property
def blocked_users(self):
# ...
return blocksUser#get_user_blocks를 User#blocked_users로 다시 이름지었다. 또, get_xxx의 경우 파이썬의 @property를 이용해 바꿀 수 있기에 그렇게 변환했다.
이 때 IDE의 기능을 이용하면 편리하다.
QuerySet 메소드 추출
그 다음은
blocks = self.user_blocks.values(
"nickname",
user_id=F("id"),
issued_date=F("blocked_blocks__issued_date"),
)위 블록을 고칠 차례이다. 위 구현은 지나치게 저수준이다. 쿼리셋으로 옮겨주자.
from django.db import models
from django.db.models import F
from django.core.cache import cache
class UserQuerySet(models.QuerySet):
def block_info(self):
return self.values(
"nickname",
user_id=F("id"),
issued_date=F("blocked_blocks__issued_date"),
)
class User(models.Model):
# ...
objects = UserQuerySet.as_manager()
#...
@property
def blocked_users(self):
data = cache.get(f"{self}:user-blocks")
if data is not None:
return data
blocks = self.user_blocks.block_info()
cache.set(f"{self}:user-blocks", blocks)
return blocks좀 더 깔끔해졌다.
읽기 쉬운 Cache Logic
위 User#blocked_users는 캐시 로직을 갖고 있다. 이 로직은 단순히 꺼낸 값이 None인지 아닌지 확인하는 것인데, Django는 이걸 위해 get_or_set이란 함수를 제공한다. 이걸 활용해 다음과 같이 써 보자.
class User:
# ...
@property
def blocked_users(self):
return cache.get_or_set(f"{self}:user-blocks", self.user_blocks.block_info())레디스 키 스키마도 하드코딩 대신 메소드로 만들어주자.
class User:
# ...
@property
def blocked_users(self):
return cache.get_or_set(self._blocked_user_key, self.user_blocks.block_info())
@property
def _blocked_user_key(self):
return f"{self}:user-blocks"캐시 키의 경우 외부로 노출하고 싶은 대상은 아니기 때문에 protected로 만들었다.
자료구조를 활용하기
이제 did_block을 바꿔보자. did_block이 하는 일은 다음과 같다.
- 차단한 유저들의 딕셔너리를 받는다.
- 인수로 받은
user_id가 이 중 있는지 확인한다.
2번 작업을 빠르게 하려면 어떻게 해야할까? Set을 도입하는게 깔끔할 거 같다. 다음과 같이 UserQuerySet을 수정하자.
class UserQuerySet(models.QuerySet):
def block_info(self):
return self._annotate_blocked_at().only("id", "nickname", "blocked_at")
def _annotate_blocked_at(self):
return self.annotate(blocked_at=F("blocked_blocks__issued_date"))
@property
def ids(self):
return {user.id for user in self}block_info 역시 리팩토링했다. 기존에는 values를 이용해 딕셔너리를 리턴했지만 이제 Model을 리턴한다.
총 결과는 다음과 같다.
# models/user.py
from django.db import models
from django.db.models import F
from django.core.cache import cache
class UserQuerySet(models.QuerySet):
def block_info(self):
return self._annotate_blocked_at().only("id", "nickname", "blocked_at")
@property
def ids(self):
return {user.id for user in self}
# private
def _annotate_blocked_at(self):
return self.annotate(blocked_at=F("blocked_blocks__issued_date"))
class User(models.Model):
nickname = models.CharField(max_length=25, unique=True)
user_blocks = models.ManyToManyField(
"user.User",
through="UserBlock",
blank=True,
through_fields=("user", "blocked_user"),
)
objects = UserQuerySet.as_manager()
class Meta:
verbose_name = "유저"
def did_block(self, user_id):
return user_id in self.blocked_users.ids
@property
def blocked_users(self):
return cache.get_or_set(self._blocked_user_key, self.user_blocks.block_info())
# private
@property
def _blocked_user_key(self):
return f"{self}:user-blocks"이제 훨씬 잘게 쪼개진, 구체적인 구현을 뒤로 숨긴 코드를 얻을 수 있다.
참고문헌
Refactoring(Martin Folwer)
즐기는 거야
좋다...