FEAT: pos kpi 상세 검색 기능 수정 #18

tasddc1226 · 2022-05-04T09:27:53Z

:: 최근 작업 주제

:: 구현 목표

여러번 쿼리를 날리는 것을 수정하였어요.
시리얼라이져에 있던 validate 함수들을 따로 파일에 분리시켰어요.

:: 구현 사항 설명

현재 작성된 api를 Q를 사용해서 리팩토링 해보았습니다.
그룹에 대한 쿼리는 TODO로 주석처리 해놓았습니다.

:: ISSUE

tasddc1226

저녁 식사로 인한 자리비움 예정..

tasddc1226 · 2022-05-04T09:28:44Z

kpi/serializers.py

+            "price",
+            "number_of_party",
+            "payment",
+        )


필드들을 수정했어요 우선 그룹은 제외하였습니다.

tasddc1226 · 2022-05-04T09:29:14Z

kpi/serializers.py

-        """
-        if large_val < small_val:
-            raise ValidationError(f"{large_val} >= {small_val} 조건을 만족해야 합니다.")
-


serializers에 검증하는 함수를 제거했어요.

Validation부분은 그대로 둬 주셨으면 좋겠습니다. 이것이 serializer에 들어있는 것과 따로 파일로 분리되는것의 어떠한 이점이 있는지 명확하지 않다면 다시 코드를 재수정하는 수고를 하고 싶지 않습니다. 여기서 수고로움은 단순히 1번문제만 바꾸는 것 뿐 아니라 2 3번 문제에도 동일하게 적용을 해야하기 때문이기도 합니다.

filter에 적용된 전반적인 if 로직은 그대로 두는게 어떤지요. validation이 과하가 들어갔다고도 생각할 수도 있지만, validation이 시스템 속도에 영향을 주고있지는 않다는 의견입니다.

하나의 예로 현재 validate.py에 is_equal_or_larger_size라는 함수가 있습니다.
이를 사용하기 위해서는 from .validate import *처럼 파일을 불러온 후,
is_equal_or_larger_size(self, start_time, end_time)처럼 사용이 가능합니다.

하지만 시리얼라이즈에 validation 함수들이 나열되어 있다면 사용하려면 RestaurantKpiSerializer.is_equal_or_larger_size(self, start_time, end_time) 처럼 사용해야할 것입니다.
api 하나에 validation 해야하는 값들이 많은데 위와 같이 불필요한 시리얼라이저 이름이 반복될 것 같다고 생각해서
분리를 한것입니다!

불필요한 시리얼라이져 이름이 반복되는 것에 동의합니다. 그치만 현재 사용하고 있는 validation 함수들이 시스템 전체적으로 쓰이기보다 KPI를 계산하는 용도에 한정하여 쓰이기 때문에 현 시점에서는 굳이 밖으로 뺄 필요가 없다 생각했습니다. 그리고 현재는 get만 쓰긴 하지만 post나 다른 요청을 함께 쓸 때에는 serializer 자체 validate 내장함수를 쓰기도 해서 validation 함수가 두 곳으로 쪼개지기도 합니다.

만약 KPI앱이 아니라 다른 곳에서 쓰일만한 validate함수가 있다면 그건 빼는 것이 더 효율적일 것이라 생각합니다.

if not all([start_time, end_time]): return Response( {"message":"start date 또는 end date를 입력하세요."}, status = status.HTTP_400_BAD_REQUEST )

혹시 이런 부분에서 날렸다고 생각하셨는지요.. 이건 진짜 임시로 제 스타일로 한것 뿐입니다 나중에 예외처리 관련해서는 상민님 것으로 통일시킬 생각이었죠

네. 그 부분을 보며 생각했던 것 같아요. 밸리데이션을 Response를 자체적으로 만드신 것을 보고, validattion을 빼두는 것 뿐 아니라 아예 축소시켰다고 생각했었습니다. 아마 이 지점에서 '굳이' 라는 단어를 썼던 것 같아요.

네 저도 이 부분은 다시 돌려놓을 생각입니다.

아마 만드는 과정에 중간점검차 올리신 거였을텐데 저는 조금은 급하게 생각해서 이미 다 만들어놓은 상태라고 미리 판단했던 것 같네요. 이 부분은 크리티컬한 저의 실수입니다.

2번, 3번 문제가 아마 거의 동일한 문제에 구현부분만 살짝 바뀌잖아여. 혹시 작업하시면서 2번 3번까지 함께 리팩터링 해주실 수 있을까요? 그렇게 한다면 좀 더 효율적으로 진행이 되지 않을까 생각합니다.

tasddc1226 · 2022-05-04T09:30:41Z

kpi/views.py

@@ -5,139 +5,81 @@
    PaymentKpiSerializer,
    PartyNumberKpiSerializer
 )
-from django.db.models import Count, Sum
+from django.db.models import Count, Sum, Q, F


Q로 쿼리문을 연결하기 위해서 import 하였습니다.
F 객체도 마찬가지로 db에 날리는 쿼리를 줄이기 위해 사용했습니다.

https://blog.myungseokang.dev/posts/django-f-class/

https://velog.io/@jxxwon/Django-Q-%EA%B0%9D%EC%B2%B4

tasddc1226 · 2022-05-04T09:32:13Z

kpi/views.py

+            'week': TruncWeek('timestamp'),
+            'month': TruncMonth('timestamp'),
+            'year': TruncYear('timestamp')
+        }


time window를 dict로 바꿔주어 key값에는 입력으로 들어온 time-window를 의미하고,
해당 key의 value로 상민님께서 작성하셨던 if-else 구문을 수정해보았습니다.

Diction처리한 코드가 이전보다 훨 깔끔하고 좋아보입니다.

redtea89

의견 코드에 댓글로 달았습니다.

redtea89 · 2022-05-05T04:57:46Z

kpi/serializers.py

-        """
-        if large_val < small_val:
-            raise ValidationError(f"{large_val} >= {small_val} 조건을 만족해야 합니다.")
-


Validation부분은 그대로 둬 주셨으면 좋겠습니다. 이것이 serializer에 들어있는 것과 따로 파일로 분리되는것의 어떠한 이점이 있는지 명확하지 않다면 다시 코드를 재수정하는 수고를 하고 싶지 않습니다. 여기서 수고로움은 단순히 1번문제만 바꾸는 것 뿐 아니라 2 3번 문제에도 동일하게 적용을 해야하기 때문이기도 합니다.

redtea89 · 2022-05-05T05:07:41Z

kpi/views.py

+        queryset = queryset & Q(price__gte=end_price) if end_price else queryset
+        queryset = queryset & Q(number_of_party__gte=start_number_of_people) if start_number_of_people else queryset
+        queryset = queryset & Q(number_of_party__lte=end_number_of_people) if end_number_of_people else queryset
+        # TODO: group id에 대한 Q filter 적용하기


기존의 코드가 필요없는 전체조회 1번, 그리고 GET인자가 들어오지 않았음에도 필요없이 쿼리를 날리고 있었군요. 쿼리 리팩터링이 속도향상에 큰 이점이 있는 것 같습니다. 다만 쿼리문의 이점이 GET 인자가 들어왔을 때 if문을 처리하여 작동하게 하는 것은 이해가 되는데 Q와 F가 어떻게 쿼리문을 줄이는지는 아직 이해가 어렵습니다. 물론 Q와 ,F를 쓰는 코드가 이전보다 훨씬 가독성도 좋아지고 깔끔함을 유지할 수 있다는 것에는 동의하는 바입니다.

이곳 쿼리를 적용하는 부분에서 축약형 if문이 쓰였습니다. 깔끔함을 추구한다면 이것보다 더 좋은 방법은 없겠으나, 이것에 익숙하지 않은 사람을 배려하는(?)혹은 가독성을 위해 if문을 풀어서 쓰는게 어떤지 의견을 드립니다.

아 그리고 우선 70번째 줄에 price__gte=end_price가 아니라 price__lte=end_price로 수정해야 겠네요 그래서 결과가 이상하게 나온 것 같습니다. if문을 풀어서 쓴다는 것이

# before queryset = queryset & Q(price__gte=start_price) if start_price else queryset # after if start_price: queryset = queryset & Q(price__gte=start_price) else: queryset

위의 after를 말씀하시는 건가용?

글죠. 근데 이건 저처럼 축약 if문에 익숙하지 않은 사람을 위한 배려로써 어떻겠느냐는 의견입니다.

redtea89 · 2022-05-05T05:11:12Z

kpi/views.py

+            'week': TruncWeek('timestamp'),
+            'month': TruncMonth('timestamp'),
+            'year': TruncYear('timestamp')
+        }


Diction처리한 코드가 이전보다 훨 깔끔하고 좋아보입니다.

redtea89 · 2022-05-05T05:16:30Z

kpi/serializers.py

-        """
-        if large_val < small_val:
-            raise ValidationError(f"{large_val} >= {small_val} 조건을 만족해야 합니다.")
-


filter에 적용된 전반적인 if 로직은 그대로 두는게 어떤지요. validation이 과하가 들어갔다고도 생각할 수도 있지만, validation이 시스템 속도에 영향을 주고있지는 않다는 의견입니다.

redtea89 · 2022-05-05T05:24:48Z

kpi/views.py

        if not time_window in time_window_archive:
-            return Response({"message":"time-window의 입력 인자는 'HOUR','DAY','WEEK','MONTH','YEAR'중의 하나입니다."}, status=status.HTTP_400_BAD_REQUEST)
+            return Response(
+                {"message":"time-window의 입력 인자는 'hour','day','week','month','year'중의 하나입니다."},


문제에 제시된 time window 키워드가 대문자이기 때문에 대문자로 쓰는 것이 맞다는 생각입니다.

도메인은 대소문자를 구분하지 않습니다.
하지만 UX관점에서는 사용자가 어떤 경로에 접속한다고 했을 때, 대문자를 사용해야하는 번거로움이 있지 않을까요? 그래서 대게 소문자로 url이 만들어진다고 생각해서 수정하였습니다.
예를들어 모바일에서 접속한다면 시간을 입력하는데 대문자로 적어야 한다는 점을 생각해보면 될 것 같네요.

아 문제만 보고 막연하게 생각했습니다. 실제 적용상황을 생각하지 않았군요. 이건 수영님의 말씀이 맞다고 생각합니다.

FEAT: pos kpi 상세 검색 기능 수정

4ec817c

tasddc1226 commented May 4, 2022

View reviewed changes

tasddc1226 closed this May 4, 2022

redtea89 reviewed May 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: pos kpi 상세 검색 기능 수정 #18

FEAT: pos kpi 상세 검색 기능 수정 #18

tasddc1226 commented May 4, 2022

tasddc1226 left a comment

tasddc1226 May 4, 2022

tasddc1226 May 4, 2022

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

tasddc1226 May 5, 2022

redtea89 May 5, 2022

tasddc1226 May 5, 2022

redtea89 May 5, 2022

tasddc1226 May 5, 2022

redtea89 May 5, 2022

redtea89 May 5, 2022

tasddc1226 May 4, 2022 •

edited

tasddc1226 May 4, 2022

redtea89 May 5, 2022 •

edited

redtea89 left a comment

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022

tasddc1226 May 5, 2022

redtea89 May 5, 2022

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022

tasddc1226 May 5, 2022

redtea89 May 5, 2022

FEAT: pos kpi 상세 검색 기능 수정 #18

FEAT: pos kpi 상세 검색 기능 수정 #18

Conversation

tasddc1226 commented May 4, 2022

:: 최근 작업 주제

:: 구현 목표

:: 구현 사항 설명

:: ISSUE

tasddc1226 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasddc1226 May 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

redtea89 left a comment

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

redtea89 May 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

tasddc1226 May 4, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited

redtea89 May 5, 2022 •

edited