-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BE/FEAT] 엑세스 토큰 재발급(리프레시 토큰) #524
Conversation
…를 호출하도록 영양소 달성 여부를 분석하는 유스케이스 생성
…yAnalysisCacheUpdater 클래스 생성
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoginResponseDTO가 응답할 때 사용하는 게 아닌 데 리팩토링할 때 확인을 못 한 거 같네요. 감사합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 같은 데이터를 담고 있더라고 API마다 다른 DTO를 만드는 것을 선호합니다.
DTO같은 경우에는 화면에 변화에 밀접하게 관련이 있다고 생각하여 변화가 많이 있을 것으로 예상되기에 애초부터 다 따로 나누는 방식으로 개발하고 있습니다.
@hwangdaesun 예를들어 dailyMealDTO , MonthMealDTO 이런 것들은 일별 조회, 월별 조회 API가 다르기 때문에 따로 분리하는 것이 아닌 하루 먹은 Meal에 대한 정보 그룹, 월에 먹은 Meal에 대한 정보 그룹으로 dto로 따로 분리해야한다고 생각합니다. |
@LJH098 그렇게 생각할 수도 있겠네요. 저는 DailyMealDTO와 MonthlyMealDTO가 데이터 구조는 같지만, 각각의 DTO를 받아서 해당 메서드가 수행하는 역할이 다르기 때문에, 추후 변경될 가능성을 고려하여 별도로 분리하였습니다. |
PR 취소해주세요! release 브랜치로 바로 push 했네요;; |
📌 관련 이슈
closes #507
🔑 주요 변경사항
리뷰어에게
@hwangdaesun LoginResponseDTO는 뒤에 DTO가 붙여져있고 common - dto 패키지에 있는 것 을 보아 response 응답용 dto가 아닌 클래스 혹은 메소드 간 단순 정보를 담기위한 dto인 것 같은데 왜 dto 이름을 LoginResponseDTO라고 했는지 잘 모르겠습니다.
토큰을 담기위한 dto면 TokenDTO가 맞지 않을까요.
LoginResponseDTO를 토큰 재발급 할 때 사용 할 수 없어서(dto 명이 토큰 재발급과 전혀 맞지 않음 -> 가독성을 해칠 수 있음) 기존에 있는 코드를 삭제하거나 수정하는 것 보다는 ReissueDTO를 새로 만들었는데
어떠한 정보를 담기위한 dto에 행위가 들어가게 되면 같은 정보더라도 행위마다 다른 dto를 만들어야 해서 이건 아닌 것 같아 LoginResponseDTO를 TokenDTO로 수정하고 전부 TokenDTO를 사용하도록 했습니다.
결론 : 클래스 명이나 메소드 명을 지을 때 신중하게 생각하고 작성하자