[Refactoring] 중복 코드 제거2(상속)

2021. 12. 25. 15:19

이전 포스팅

2021.12.15 - [Refactoring] - [Refactoring] 중복 코드 제거1

[Refactoring] 중복 코드 제거1

2021.12.09 - [Etc] - Refactoring 일지 1. APIView To ModelViewSet Refactoring 일지 1. APIView To ModelViewSet APIView → ModelViewSet 사용하기 프로젝트를 시작할 때, 나는 기존에 자바를 썻기 때문에, 파..

jino-dev-diary.tistory.com

build_data() 메소드는 어느정도 리팩토링이 된 것 같다. (아직 부족하지만)

이번에 할 리팩토링

NoteViewSet, TopicViewSet, PageViewSet 상속받기
각 ViewSet(NoteViewSet, TopicViewSet, PageViewSet)의 create() 메소드가 매우 비슷한 로직을 가지고있다. 즉, 중복 코드가 발생한다.
또한 TopicViewSet과 PageViewSet의 list() 메소드도 로직이 매우 유사하다.
이것을 저번 포스팅에서 build_data() 메소드를 BaseData 라는 상위 클래스를 만들어 NoteData, TopicData, PageData 클래스가 상속받도록 한 것처럼 BaseContentViewSet이라는 상위 클래스를 만들어 NoteViewSet, TopicViewSet, PageViewSet 클래스가 상속받도록 해서 BaseContentViewSet에 공통 메서드를 구현하면 되지 않을까? 라는 생각으로 바로 리팩토링을 시작했다.
다형성을 이용하는 리팩토링은 여전히 익숙하지 않다. 내가 아직 초보라고 느끼고, 그나마 잘 할 수 있는 리팩토링 수준은 메소드추출, 인라인코드, 매직넘버 to 상수..? 이정도의 간단한 수준이기 때문에 다형성을 이용한 리팩토링은 좀 오래 걸릴것 같다고 겁(?)을 조금 먹었다.
그럼에도 불구하고 나에게는 테스트코드가 있다. 이것 저것 시도해보고 잘 안되면 원복하고 "아, 이 방법은 좀 더 공부가 필요하거나, 여기서는 적용하기 힘들겠다;"하고 블로그나 노트에 정리만 잘 해놓으면 된다. (비록 그 삽질동안 잡아먹는 시간은 아까울수 있지만..)
바로 리팩토링을 시작하기 전에 대충 구조를 상상했는데 다음과 같다.

계정 인증을 위한 CtrlfAuthenticationMixin은 NoteViewSet, TopicViewSet, PageViewSet 모두 필요하니까 ModelViewSet과 CtrlfAuthenticationMixin 클래스를 상속받은 BaseContentViewSet을 구현하고, NoteViewSet, TopicViewSet, PageViewSet이 BaseContentViewSet을 상속받도록 하고, create() 메소드는 셋 다 로직이 같다. (각 Model과 Issue에 대한 data를 만들고, serializer를 통해 검증 후, Model과 Issue 레코드를 생성한다) Model과 Issue에 대한 data를 만드는 로직만 Model마다 조금 다를 뿐이다. 그러므로 BaseContentViewSet에 create() 메소드를 공통 메소드로 만들고 각 하위 클래스에서 BaseContentViewSet의 create() 메소드를 호출하면 중복을 확실히 줄일 수 있을 것 같다.

이런 생각을 가지고 리팩토링을 바로 시작했다.

리팩토링 시작

1. `NoteViewSet`, `TopicViewSet`, `PageViewSet` 클래스의 상위 클래스인 `BaseContentViewSet` 생성

class NoteViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    @swagger_auto_schema(**SWAGGER_NOTE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        note_data, issue_data = NoteData().build_data(request, ctrlf_user)

        note_serializer = NoteSerializer(data=note_data)
        issue_serializer = IssueCreateSerializer(data=issue_data)

        if note_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=note_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)

     ...


class TopicViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    @swagger_auto_schema(**SWAGGER_TOPIC_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        topic_data, issue_data = TopicData().build_data(request, ctrlf_user)

        topic_serializer = TopicSerializer(data=topic_data)
        issue_serializer = IssueCreateSerializer(data=issue_data)

        if topic_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=topic_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)
        return Response(status=status.HTTP_201_CREATED)

    ...


class PageViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    @swagger_auto_schema(**SWAGGER_PAGE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        page_data, issue_data = PageData().build_data(request, ctrlf_user)

        page_serializer = PageSerializer(data=page_data)
        issue_serializer = IssueCreateSerializer(data=issue_data)

        if page_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=page_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)
     ...

각 Viewset(NoteViewSet, TopicViewSet, PageViewSet)의 create() 메소드의 로직이 전부 비슷하다.
- 각 model별로 data가 조금씩 다른 것 뿐, 그 외에는 로직이 전부 같다.
Upper 클래스를 생성하여 나머지 ViewSet이 상속받도록 변경해보자

class BaseContentViewSet(ModelViewSet):
    ...

    def create(self, request, *args, **kwargs):
        related_model_serializer = self.get_serializer(data=kwargs["model_data"])
        issue_serializer = IssueCreateSerializer(data=kwargs["issue_data"])

        if related_model_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=related_model_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)

class NoteViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = NoteSerializer

    ...

    @swagger_auto_schema(**SWAGGER_NOTE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
                ctrlf_user = self._ctrlf_authentication(request)
        note_data, issue_data = NoteData().build_data(request)
        data = {"model_data":note_data, "issue_data":issue_data}

        return super().create(request, **data)


class TopicViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = TopicSerializer

    ...

    @swagger_auto_schema(**SWAGGER_TOPIC_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        topic_data, issue_data = TopicData().build_data(request)
        data = {"model_data":topic_data, "issue_data":issue_data}

        return super().create(request, **data)


class PageViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = PageSerializer
    ...

    @swagger_auto_schema(**SWAGGER_PAGE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        page_data, issue_data = PageData().build_data(request)
        data = {"model_data":page_data, "issue_data":issue_data}

        return super().create(request, **data)

Upper 클래스인 BaseContentViewSet을 만들어 중복코드를 줄였다.
근데 ctrlf_user 의 로직도 중복코드라서 BaseContentViewSet으로 옮기고 싶다

2. `basedata.py` 수정

class BaseData:
    def __init__(self):
        pass

    def build_data(self, request, ctrlf_user):
        model_data = {
            "title": request.data["title"],
            "owners": [ctrlf_user.id],
        }
        issue_data = {
            "owner": ctrlf_user.id,
            "title": request.data["title"],
            "reason": request.data["reason"],
            "status": CtrlfIssueStatus.REQUESTED,
            "action": CtrlfActionType.CREATE,
        }
        return model_data, issue_data


class NoteData(BaseData):
    def __init__(self):
        super().__init__()

    def build_data(self, request, ctrlf_user):
        note_data, issue_data = super(NoteData, self).build_data(request, ctrlf_user)
        issue_data["related_model_type"] = CtrlfContentType.NOTE

        return note_data, issue_data


class TopicData(BaseData):
    def __init__(self):
        super().__init__()

    def build_data(self, request, ctrlf_user):
        topic_data, issue_data = super(TopicData, self).build_data(request, ctrlf_user)
        topic_data["note"] = request.data["note_id"]
        issue_data["related_model_type"] = CtrlfContentType.TOPIC

        return topic_data, issue_data


class PageData(BaseData):
    def __init__(self):
        super().__init__()

    def build_data(self, request, ctrlf_user):
        page_data, issue_data = super(PageData, self).build_data(request, ctrlf_user)
        page_data["topic"] = request.data["topic_id"]
        page_data["content"] = request.data["content"]
        issue_data["related_model_type"] = CtrlfContentType.PAGE
        return page_data, issue_data

ctrlf_user 처리를 BaseContentViewSet에서 하기 위해 basedata.py에서 삭제하자.

# basedata.py
from rest_framework.request import Request
from .models import CtrlfActionType, CtrlfContentType, CtrlfIssueStatus

class BaseData:
    request: Request

    def __init__(self, request):
        self.request = request

    def build_data(self):
        model_data = {
            "title": self.request.data["title"],
        }
        issue_data = {
            "title": self.request.data["title"],
            "reason": self.request.data["reason"],
            "status": CtrlfIssueStatus.REQUESTED,
            "action": CtrlfActionType.CREATE,
        }
        return model_data, issue_data


class NoteData(BaseData):
    def build_data(self):
        note_data, issue_data = super().build_data()
        issue_data["related_model_type"] = CtrlfContentType.NOTE

        return {"model_data": note_data, "issue_data": issue_data}


class TopicData(BaseData):
    def build_data(self):
        topic_data, issue_data = super().build_data()
        topic_data["note"] = self.request.data["note_id"]
        issue_data["related_model_type"] = CtrlfContentType.TOPIC

        return {"model_data": topic_data, "issue_data": issue_data}


class PageData(BaseData):
    def build_data(self):
        page_data, issue_data = super().build_data()
        page_data["topic"] = self.request.data["topic_id"]
        page_data["content"] = self.request.data["content"]
        issue_data["related_model_type"] = CtrlfContentType.PAGE

        return {"model_data": page_data, "issue_data": issue_data}

각 ModelViewSet의 create() 메소드에서 request를 인자로 받도록 했다.
이에 따라 ModelViewSet의 create() 메소드도 조금 수정했다.

3. `create()` 메소드 수정

class BaseContentViewSet(ModelViewSet):
    ...

    def create(self, request, *args, **kwargs):
        related_model_serializer = self.get_serializer(data=kwargs["model_data"])
        issue_serializer = IssueCreateSerializer(data=kwargs["issue_data"])

        if related_model_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=related_model_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)

class NoteViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = NoteSerializer

    ...

    @swagger_auto_schema(**SWAGGER_NOTE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
                ctrlf_user = self._ctrlf_authentication(request)
        note_data, issue_data = NoteData().build_data(request)
        data = {"model_data":note_data, "issue_data":issue_data}

        return super().create(request, **data)


class TopicViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = TopicSerializer

    ...

    @swagger_auto_schema(**SWAGGER_TOPIC_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        topic_data, issue_data = TopicData().build_data(request)
        data = {"model_data":topic_data, "issue_data":issue_data}

        return super().create(request, **data)


class PageViewSet(CtrlfAuthenticationMixin, BaseContentViewSet):
    serializer_class = PageSerializer
    ...

    @swagger_auto_schema(**SWAGGER_PAGE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        page_data, issue_data = PageData().build_data(request)
        data = {"model_data":page_data, "issue_data":issue_data}

        return super().create(request, **data)

basedata.py의 build_data() 메소드를 수정함에 따라 각 ModelViewSet에서 data를 구성하는 로직도 수정하고, ctrlf_user 로직 부분도 BaseContentViewSet으로 옮기자.

class BaseContentViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        related_model_serializer = self.get_serializer(data=kwargs["model_data"])
        issue_serializer = IssueCreateSerializer(data=kwargs["issue_data"])

        if related_model_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=related_model_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)

class NoteViewSet(BaseContentViewSet):
    serializer_class = NoteSerializer

    ...

    @swagger_auto_schema(**SWAGGER_NOTE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        data = NoteData(request).build_data()
        return super().create(request, **data)


class TopicViewSet(BaseContentViewSet):
    serializer_class = TopicSerializer

    ...

    @swagger_auto_schema(**SWAGGER_TOPIC_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        data = TopicData(request).build_data()
        return super().create(request, **data)


class PageViewSet(BaseContentViewSet):
    serializer_class = PageSerializer
    ...

    @swagger_auto_schema(**SWAGGER_PAGE_CREATE_VIEW)
    def create(self, request, *args, **kwargs):
        data = PageData(request).build_data()
        return super().create(request, **data)

4. `if else` statement 삭제

class BaseContentViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        kwargs["model_data"]["owners"] = [ctrlf_user.id]
        kwargs["issue_data"]["owner"] = ctrlf_user.id
        related_model_serializer = self.get_serializer(data=kwargs["model_data"])
        issue_serializer = IssueCreateSerializer(data=kwargs["issue_data"])

        if related_model_serializer.is_valid() and issue_serializer.is_valid():
            issue_serializer.save(related_model=related_model_serializer.save())
        else:
            return Response(status=status.HTTP_400_BAD_REQUEST)

        return Response(status=status.HTTP_201_CREATED)

if else를 굳이 사용할 필요가 없다. indent만 불편해지니까 삭제하자

class BaseContentViewSet(CtrlfAuthenticationMixin, ModelViewSet):
    ...

    def create(self, request, *args, **kwargs):
        ctrlf_user = self._ctrlf_authentication(request)
        kwargs["model_data"]["owners"] = [ctrlf_user.id]
        kwargs["issue_data"]["owner"] = ctrlf_user.id
        related_model_serializer = self.get_serializer(data=kwargs["model_data"])
        issue_serializer = IssueCreateSerializer(data=kwargs["issue_data"])

        related_model_serializer.is_valid(raise_exception=True)
        issue_serializer.is_valid(raise_exception=True)
        issue_serializer.save(related_model=related_model_serializer.save())

        return Response(status=status.HTTP_201_CREATED)

is_valid() 메소드에서 exception이 발생할경우 400_BAD_REQUEST를 리턴하도록 exceptions.py에 구현되어있었기 때문에 여기서 굳이 구현할 필요가 없다.

커밋

후기

커밋을 보면 create() 메소드의 로직을 BaseContentViewSet으로 옮기는 커밋에 list() 메소드의 리팩토링도 포함되어있다. 커밋 메시지를 더 자세하게 쓰던지, 커밋 단위를 자르던지 하는 연습이 필요할듯
코드를 바로 작성하기전에 위 그림처럼 어느정도 머리속이나 낙서 수준으로 구조화 해놓고 구현하면 그렇게 하지 않을 때 보다 더 코드가 잘 짜지는것 같다. 좋은 습관일듯
물론 내 테스트가 완벽하지는 않지만, 테스트가 있음으로 좀 더 과감하게 리팩토링 할 수 있다는걸 느꼈다. 하다가 실패하면 다시 이전 코드로 돌아가면 그만이다.

저작자표시

'Refactoring' 카테고리의 다른 글

[Refactoring] 중복 코드 제거1 (0)	2021.12.15
[Refactoring] APIView To ModelViewSet (0)	2021.12.09

Dev 일기장