Skip to content
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

✨ [STMT-169] 스터디 완료 기능 구현 #122

Merged
merged 3 commits into from
May 20, 2024

Conversation

05AM
Copy link
Collaborator

@05AM 05AM commented May 6, 2024

💁 해결 하려는 문제를 적어주세요

  • 스터디 완료를 할 수 있다.

🤔 어떤 방식으로 해결했는지 적어주세요

  • 구현되어 있는 기능으로 스터디 완료 기능을 구현했습니다.
  • study period에 도메인 로직을 추가했습니다.

🙋 중점적으로 리뷰 했으면 하는 부분이 있다면 적어주세요

  • study 내부에서 오늘 날짜로 today라는 LocalDate 값을 생성해서 검사하는데, 랜덤 값 때문에 테스트가 어려워질 수도 있을 것 같습니다.
  • usecase나 study 도메인에 finish 메서드에 LocalDate를 인수로 줄지 고민중인데 내부에서 처리하는게 로직상 좋아보여서 고민이 됩니다.
  • 그런데 그냥 LocalDate.now()를 mock 처리하면 될까요?

🧑‍🏫 이해를 위해 필요한 자료가 있다면 첨부해주세요

  • 이전에 study_history라는 테이블을 따로 만들기로 설계했던 것 같은데, 다시 고민해보니 study에 이미 isFinished 필드가 있고, study_history와 필드값도 거의 차이가 없어서 해당 테이블을 삭제하기로 결정했습니다! 이에 대해 이견이 있으시다면 편하게 말씀해주시면 감사하겠습니다 😊

@05AM 05AM added the D-3 3일 전 까지 리뷰해주세요 label May 6, 2024
@05AM 05AM self-assigned this May 6, 2024
@05AM 05AM requested a review from zxcv9203 as a code owner May 6, 2024 11:36
@05AM 05AM changed the title Stmt 169 admin study complete api [STMT-169] 스터디 완료 기능 구현 May 6, 2024
@github-actions github-actions bot added D-2 2일 전 까지 리뷰해주세요 and removed D-3 3일 전 까지 리뷰해주세요 labels May 6, 2024
@github-actions github-actions bot added D-1 1일 전 까지 리뷰해주세요 and removed D-2 2일 전 까지 리뷰해주세요 labels May 7, 2024
@zxcv9203
Copy link
Collaborator

zxcv9203 commented May 8, 2024

  • study 내부에서 오늘 날짜로 today라는 LocalDate 값을 생성해서 검사하는데, 랜덤 값 때문에 테스트가 어려워질 수도 있을 것 같습니다.
  • usecase나 study 도메인에 finish 메서드에 LocalDate를 인수로 줄지 고민중인데 내부에서 처리하는게 로직상 좋아보여서 고민이 됩니다.
  • 그런데 그냥 LocalDate.now()를 mock 처리하면 될까요?

내부에 있는 LocalDate.now()를 Mocking하신다는 말씀이 맞으실까요?
제 기억으로 스태틱 메서드를 테스트할 때 static Mock 같은것을 해야 Mocking이 되었던 걸로 기억하는데 해당 방법은 바이트코드를 직접 조작하기 때문에 퍼포먼스 저하도 발생하고 추가적인 코드들이 필요해 안티패턴으로 불리는걸로 알고 있습니다.. 🤔

찬미님 말씀대로 내부에서 처리하면 인수도 없고 깔끔하다는 생각도 충분히 공감이 되는 것 같습니다.
하지만 우리가 정상적으로 코드가 동작하는지 확인하려면 테스트는 필수적이고 테스트를 통해 안정적인 애플리케이션을 만들어 나갈 수 있다고 생각하는데 작성한 코드가 보기에는 예뻐도 테스트가 어려워진다면 코드에서 좋지 않은 상황이라는 경고를 보내는 것이라고 생각합니다.

그래서 저는 파라미터로 받는게 좋다는 생각이 듭니다. (지금 같은 경우는 외부로 부터 값을 받기만 하면 해결 될 상황이라 Clock이나 별도 인터페이스로 만들지는 않아도 된다고 생각이 드네요..!)

  • 이전에 study_history라는 테이블을 따로 만들기로 설계했던 것 같은데, 다시 고민해보니 study에 이미 isFinished 필드가 있고, study_history와 필드값도 거의 차이가 없어서 해당 테이블을 삭제하기로 결정했습니다! 이에 대해 이견이 있으시다면 편하게 말씀해주시면 감사하겠습니다 😊

넵!!! 알겠습니다 👍

@github-actions github-actions bot added D-0 바로 리뷰가 필요해요 and removed D-1 1일 전 까지 리뷰해주세요 labels May 8, 2024
@05AM 05AM changed the title [STMT-169] 스터디 완료 기능 구현 ✨ [STMT-169] 스터디 완료 기능 구현 May 9, 2024
@05AM
Copy link
Collaborator Author

05AM commented May 12, 2024

넵 자세한 답글 감사합니다! 👍👍👍

해당 내용 참고하여 이전 답글에 적은 내용대로 구현하겠습니다.

@05AM 05AM merged commit e6d1dd7 into dev May 20, 2024
1 check passed
@05AM
Copy link
Collaborator Author

05AM commented May 20, 2024

앗 리뷰 반영한 커밋들을 push 하는것을 잊었네요

다시 작업하고 pr 올리겠습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-0 바로 리뷰가 필요해요
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants