Skip to content

Conversation

@whxogus215
Copy link
Contributor

@whxogus215 whxogus215 commented Oct 6, 2024

🔧연결된 이슈

  • closed

🛠️작업 내용

  • UserControllerUserService만 의존하도록 변경
  • UserServicePasswordEncoder 보다 더 추상화 된 객체에 의존하도록 변경
  • user 도메인에 대해서 도메인형 패키지로 변경
  • UserService에 대한 테스트 코드 추가

🤷‍♂️PR이 필요한 이유

이번 코드 작업의 주된 목적은 Testabality, 프로그램의 테스트 가능성 증가입니다. 따라서 테스트하기 쉬운 코드로 변경하는 것을
목적으로 설계 구조를 변경하였습니다. user를 시작으로 점차적으로 개선을 할 예정이며, 추후 선후수 체크, 특정학과의 세부요건 체크 등 비즈니스 요구사항도 추가할 계획입니다.


AS-IS

  • UserControllerUserServiceUserDao에 의존함으로써 사용자 요청 값을 처리하는 컨트롤러의 역할 그 이상을 담당하고 있습니다. (SRP 원칙에 위반)
  • UserServicePasswordEncoder라는 시큐리티의 인터페이스에 의존하고 있습니다. SecurityConfig를 확인해 본 결과, BCryptPasswordEncoder를 주입받고 있습니다. 즉, UserService의 메서드를 테스트하려면 BCryptPasswordEncoder를 주입받아야 하는데, 이는 UserService를 예측 가능한 범위 내에서 테스트하지 못하게 만듭니다.

TO-BE

  • UserControllerUserDao에게 메시지를 전달하는 책임을 UserService에게 몰아주었습니다. UserController는 사용자 요청 값에 대한 처리만 하게 됩니다.
    • MajorsDao를 여전히 의존하고 있지만 이는 major 도메인 리팩토링 시 개선해보겠습니다.
  • UserServicePasswordEncoder 보다 더 추상화 된 UserEncoder를 의존하고 있습니다. 이로써 UserService를 테스트하기 위해 필요한 UserEncoder의 Fake 객체를 만들 수 있게 되었습니다.
    • 테스트 시에는 BCryptPasswordEncoder가 아닌 FakeUserEncoder라는 테스트용 인코더를 사용하도록 합니다. 따라서 UserService의 테스트 코드에는 UserService가 갖고 있는 메서드에 대한 테스트에 집중할 수 있습니다.

레거시 코드를 리팩토링하는 것이다보니 변경사항이 많습니다 😢 따라서 user 도메인 부분 위주로 확인하시는게 좋을 것 같습니다. 천천히 보시고, 궁금한 점은 질문해주세요!

변경한 패키지 구조

├── user
│ ├── controller
│ ├── service
│ ├── domain
│ └── infrastructure

✔️PR 체크리스트

  • 필요한 테스트를 작성했는가?
  • 다른 코드를 깨뜨리지 않았는가?
  • 연결된 이슈 외에 다른 이슈를 해결한 코드가 담겨있는가?

- UserService에 updatePassword, getByStudentId 메서드 추가
- 커스텀 예외 UserNotFoundException 추가
- 개발 초기에 만들었고, 아예 사용 안되는 클래스라서 삭제함
- UserService가 UserEncoder 인터페이스에 의존
- UserEncoder를 구현한 구현체가 PasswordEncoder를 의존
- UserDao -> UserRepository
- FakeUserEncoder에서 재정의한 matches 메서드 오류 수정
@whxogus215 whxogus215 added 🔧 기능 추가 새로운 기능 추가 🔨 리팩토링 리팩토링 서비스 서비스 로직 관련 이슈 labels Oct 6, 2024
@whxogus215 whxogus215 requested a review from gmltn9233 October 6, 2024 06:07
@whxogus215 whxogus215 self-assigned this Oct 6, 2024
Copy link
Contributor

@gmltn9233 gmltn9233 left a comment

Choose a reason for hiding this comment

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

덕분에 저도 헷갈렸던 개념들을 다시 찾아보며 생각할수있었던것 같습니다👍
User 도메인 뿐만 아니라 앞으로 도메인형 패키지 구조로 점차 변경해나갈 계획인가요??

@@ -0,0 +1,591 @@
INSERT INTO major (major_id, major)
Copy link
Contributor

Choose a reason for hiding this comment

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

sql 문을 추가한게 테스트 DB와 개발 DB를 분리하기 위함인가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.gitignore에서 .sql을 제거했더니 같이 커밋된 것 같습니다.
추후 리팩토링을 통해 점차 단위 테스트를 늘리고, 통합 테스트의 비중이 줄어들다보면
테스트 DB와 개발 DB를 분리할 수 있을 것 같았어요.
sql문을 활용한 테스트는 공학인증 검사 리팩토링 후, 테스트 과정에서 사용될 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

테스트코드에서는 PasswordEncoder에 의존하지않는FakeUserEncoder 구현체를 사용하면서 테스트 용이성을 높인거군요!! 좋습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

도메인형 패키지 구조로 변경하는 과정에서 Dao에서 Repository로 변경한 것으로 짐작되는데 두 가지의 차이를 어떤 점이라고 생각하는지 궁금합니다! 저는 아직 체감이 잘 안되어서요.. 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 생각한 Dao와 Repository의 차이점은 다음과 같습니다.

  • Dao : 영속성 계층에 포함되어 있으며, DB 접근과 관련된 로직(SQL)이 포함되어 있는 객체 (= 단순히 DB와 서비스 계층의 매개 역할)
  • Repository : 도메인 계층에 포함되어 있으며, 도메인 객체를 컬렉션(List, Set 등)처럼 가져올 수 있는 객체

현재로서는 Repository 보다는 Dao에 가까운 것 같습니다. 다만, 공학인증 검사 로직을 개선하면서
비즈니스 로직을 가진 도메인 객체를 추가하다보면 Dao 보다는 Repository에 좀 더 가까워질 것 같았습니다.

참고 ) https://tecoble.techcourse.co.kr/post/2023-04-24-DAO-Repository/

Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 자료 추천 감사합니다~ 점점 리팩토링하면서 고쳐나가야할 과제겠네요

Copy link
Contributor Author

@whxogus215 whxogus215 left a comment

Choose a reason for hiding this comment

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

PR 단위가 생각보다 커져서 리뷰하실 때, 시간이 많이 걸렸을 것 같습니다 😢
다음 PR에는 조금 줄여보겠습니다.

@gmltn9233
Copy link
Contributor

PR 단위가 생각보다 커져서 리뷰하실 때, 시간이 많이 걸렸을 것 같습니다 😢 다음 PR에는 조금 줄여보겠습니다.

그래도 흐름은 한 흐름인것 같아서 괜찮았습니다! 시간이 많이 걸린건 갑자기 바쁜일이 몰려서..😥
수고하셨습니다~👍

@gmltn9233 gmltn9233 merged commit 47fffe3 into develop Oct 28, 2024
@whxogus215 whxogus215 deleted the refactor/UserService branch November 18, 2024 07:41
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.

3 participants