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

[REFACTOR] 메일 도메인 리팩토링 #150

Merged
merged 23 commits into from
Oct 4, 2024

Conversation

paragon0107
Copy link
Contributor

✨ Related Issue

📝 기능 구현 명세

  • 이곳에는 postman 테스트 결과를 넣어주세요

image

image

🐥 추가적인 언급 사항

  • 메일 관련해서 리팩토링 해봤습니다. 기존 MailService 에 너무 많은 기능과 서비스단에 들어가지 말아야 할 기능들이 들어가 있어서 따로 빼주었습니다. 또한 메일이라는 도메인 이름이 다시 생각해보니 부적절한 것 같아 도메인 이름을 변경했습니다
  • 추가적으로 프론트 단으로 부터 받는 데이터 형식이 address에서 email로 바꼈는데 기존을 유지할 지 프론트에게 말하고 바꿔야 할 지 의견 여쭙니다

@paragon0107 paragon0107 linked an issue Sep 20, 2024 that may be closed by this pull request
4 tasks
@Chan531 Chan531 self-requested a review September 20, 2024 08:00
Copy link
Contributor

@Chan531 Chan531 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다!

앱잼 때부터 지속적으로 같은 리뷰를 드리고 있는데 2차 스프린트에서는 이 점이 꼭 고쳐지면 좋겠네요.

  • 클래스 형식 (줄바꿈, 띄어쓰기 등)
  • pr 올릴 때 옆에 Reviewers와 Reviewers, Labels 할당

그리고 현재 하신 작업이 chore보단 refactor에 가까운 거 같아요.
똑같이 느끼신다면 pr 네이밍 바꿔주시면 좋을 거 같아요.

데이터 형식이 address에서 email로 바뀌었다고 말씀해주셨는데 클라이언트랑 논의 후 생각하시는 방향으로 수정하시면 될 거 같습니다.

마지막으로 EmailVerification이라는 네이밍으로 컨트롤러, 서비스 등을 만들어주셨는데요!

이메일 검증 뿐만 아니라 메일 전송의 일도 하고 있어서 그냥 Email이라는 네이밍을 사용하는 것이 나아보이는데 어떻게 생각하시는지 답변 남겨주세요~

@paragon0107 paragon0107 added the feat 기능 label Sep 20, 2024
Copy link
Contributor Author

@paragon0107 paragon0107 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍에 관련해서 말씀드리면 이 도메인의 주요 역할이 이메일을 검증이라 생각합니다. 이메일을 전송하는 일은 이메일을 검증하기에 있어서 부가적인 일이라 판단하여 이메일 검증에 초점을 두었습니다. 이메일이 넓은 범위를 포함하고는 있지만 보다 명확한 역할을 명시해주는게 좋을 것 같아 다음과 같이 변경하였습니다!

@paragon0107 paragon0107 changed the title [CHORE] 메일 도메인 리팩토링 [REFACTOR] 메일 도메인 리팩토링 Sep 21, 2024
@Chan531
Copy link
Contributor

Chan531 commented Sep 21, 2024

보다 명확한 역할은 메소드 네이밍을 보고 파악하는 것만으로도 충분하지 않을까요?!

저는 Email이라는 도메인 안에 이메일 전송과 이메일 검증이라는 api가 존재하고 이를 나타내는 것은 메소드 명으로도 충분하다 생각합니다.

만약 이메일 검증만을 한다면 민규님이 작성해주신대로 해도 괜찮을 거 같은데 이메일 전송이라는 역할이 있기 때문에 도메인 네이밍을 이메일 검증이라는 뜻으로 한정지을 이유는 없다고 생각합니다.

코멘트 보시고 민규님이 맞다고 생각해주시는 방향대로 코드 작성해주시고 머지합시다~

@paragon0107
Copy link
Contributor Author

저는 이 EmailVerification 도메인에 속한 엔티티가 정말 이메일이라는 값을 저장하기 위한 용도가 아니라 이메일로 발송된 코드를 저장하기 때문에 이메일이라는 도메인 네임보다는 EmailVerification이라는 네이밍이 해당 도메인을 좀 더 명확하게 설명을 한다고 생각합니다. 또한 기존 이메일이라는 객체가 따로 있기 때문에 해당 도메인은 인증을 검증하는 도메인으로 생각하여 처리했습니다

@Chan531
Copy link
Contributor

Chan531 commented Sep 23, 2024

넵 확인했습니다 EmailVerification이라는 도메인 네이밍을 사용하실거라면 패키지 네이밍 컨벤션도 지켜주세요.
패키지 네이밍에는 대문자가 들어가면 안됩니다.

@Chan531 Chan531 added refactor and removed feat 기능 labels Sep 24, 2024
@paragon0107 paragon0107 merged commit f960b45 into develop Oct 4, 2024
1 check passed
@paragon0107 paragon0107 deleted the chore/#147-auth-member-mail-team-refactor branch October 4, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CHORE] Auth, Member, Mail, Team 부분 리팩토링
2 participants