feature : #4 개인정보 수집 구현 by Koonayoung518 · Pull Request #9 · 1sTeam/all-knu-auth

Koonayoung518 · 2024-02-18T15:23:13Z

헥사고날 아키텍처 적용했습니다.
개인정보 수집 동의 여부에 따라 저장할 값을 확인하기 위해 knuVeriusApiService.getStudentInfo()의 반환값을 Map에서 VeriusInfo로 변경했습니다.

기능 구현을 마무리한 후 exception 클래스가 계속 늘어나 가독성이 떨어지는 것 같아 개선하는 시간을 가지면 좋을 것 같습니다.

suhongkim98

헥사고날 아키텍처로 작성하셔서 고생 많으셨습니다.
일단 1차적으로 큰 부분만 리뷰해드렸어요, 나머지는 추후에 또 봐드리도록 할게요

suhongkim98 · 2024-02-18T15:41:17Z

+    @Id
+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+    @Column(name = "is_name", nullable = false)


is_name, is_student_id 처럼 수집하고자 하는 항목마다 컬럼을 추가해야하는 구조는 확장성에 좋아보이지 않아요.
student와 consent는 1대다 관계로 가지는 구조로 하는게 어떨까요?

studentId

consentType

value?

...

해당 구조로 반영했습니다. 관련 질문이 있습니다.
updateConsentService.updateConsents()에서 동의 여부(consent)에 따라 student에 저장하는 값이 다른데,
updateStudentPort.updateStudent(Student.withId(student.getId(), veriusInfo.getStudentId(updateConsentByType(student, ConsentType.STUDENT_ID, command.isStudentId())), veriusInfo.getMajor(updateConsentByType(student, ConsentType.MAJOR, command.isMajor())), veriusInfo.getName(updateConsentByType(student, ConsentType.NAME, command.isName()))));

이 부분이 타입이 늘어나면 서비스를 바꿔야해 확장성 면에서 안좋다고 생각합니다. 더 좋은 방향이 있을지 궁금합니다..

아래부분에서 멘트 드렸습니당.

suhongkim98 · 2024-02-18T15:46:02Z

+    public VeriusInfo getStudentInfo(Map<String, String> veriusCookies) {
+        //참인재시스템에서 학과, 학번, 이름 등 학생 정보를 긁어다 준다.
+        //해당 참인재 쿠키로 정보를 긁어온다.
+        String url = apiEndpointSecretProperties.getCrawling().getVeriusStudentInfo() + "?CURRENT_MENU_CODE=MENU0028&TOP_MENU_CODE=MENU0017";


이 로직은 adapter의 out 영역으로 이동시킨 후 port를 통해 가져와 유즈케이스에서 사용하도록 변경해주세용

suhongkim98

2차적으로 큰 부분만 리뷰해드렸어요, 유즈케이스부분은 수정이 좀 많이 필요해보입니다.
전체적인 유즈케이스 로직 흐름을 다음과 같이 해주세요

조회
검증
persist

suhongkim98 · 2024-02-19T11:56:53Z

+
+    @Override
+    @Transactional
+    public void updateConsents(String studentId, UpdateConsentCommand command) {


현재 로직은 중간중간 updateStudentPort를 이용해 DB에 접근하여 갱신을 해주는데 이는 결국 데이터베이스 주도 개발을 한다는 의미와 같아요. 이와 같은 구조는 수집하고자 하는 항목이 변할 때마다 port를 수정해주어야 해서 확장성면에서 좋지 않습니다.

UpdateConsentUseCase의 역할은 단순합니다.

로직

studentId를 기준으로 수집 동의 여부를 조회한다.

학번 수집에 거부했다면 예외를 발생시킨다.

studentId로 Student를 찾는다.

수집 동의한 항목만 Student 도메인 엔티티를 수정한다. (학번, 전공, 이름)

수정된 Student 도메인 엔티티를 updateStudentPort를 통해 갱신한다. 예시) updateStudentPort.updateStudent(student);

현재처럼 updateStudentPort를 중간중간 호출하지 말고 updateStudentPort를 유즈케이스에서 마지막에 단 한번만 호출하도록 로직을 수정해보시겠어요?

suhongkim98 · 2024-02-19T11:59:19Z

+    comments = "version: 1.5.3.Final, compiler: javac, environment: Java 21.0.1 (Oracle Corporation)"
+)
+@Component
+public class ConsentMapperImpl implements ConsentMapper {


Mapstruct가 자동으로 만들어주는 구현체는 gitignore 대상이에요.

src/main/generated..`는 gitignore에 추가해주세요

아직 이그노아가 안된 것 같아용

suhongkim98 · 2024-02-19T12:01:45Z

+                veriusInfo.getName(updateConsentByType(student, ConsentType.NAME, command.isName()))));
+    }
+
+    private void validateStudentId(String studentId, VeriusInfo veriusInfo) {


이 메서드는 재사용성이 없어보여요, 메서드로 빼는 작업은 오히려 가독성에 해가 될 수 있을 것 같아요.

suhongkim98 · 2024-02-19T12:02:35Z

+        }
+    }
+
+    private Student findStudent(String studentId) {


이 메서드도 재사용할 일이 없어보입니당.
student가 없으면 updateStudentPort를 호출하는데 이것도 하지 않도록 로직 수정해봐주세용

suhongkim98

2차적으로 보이는 큰 부분만 리뷰해드렸어요~!

suhongkim98 · 2024-03-06T10:28:56Z

+    comments = "version: 1.5.3.Final, compiler: javac, environment: Java 21.0.1 (Oracle Corporation)"
+)
+@Component
+public class ConsentMapperImpl implements ConsentMapper {


아직 이그노아가 안된 것 같아용

suhongkim98 · 2024-03-06T10:32:20Z

+            Consent consent = updateConsentByType(student, type, command.getInfoByConsentType(type));
+            student.updateInfo(type, getVeriusInfo(veriusInfo, consent));
+        }
+        updateStudentPort.updateStudent(student);


suhongkim98 · 2024-03-06T10:33:39Z

+        }
+        Student student = loadStudentPort.loadStudentByStudentId(studentId);
+        if (student == null) {
+            student = Student.withoutId(studentId, null, null);


withoutId 처럼 매번 생성자를 만들어야하는 구조는 확장성에 어려움이 있을 것 같아요
빌더 쓰는게 어떨까여?

suhongkim98 · 2024-03-06T10:36:52Z

+        return new Consent(id, type, consent, student);
+    }
+
+    public static Consent withoutId(ConsentType type, boolean consent, Student student) {


이펙티브 자바 아이템2. 생성자에 매개변수가 많다면 빌더를 고려하라 한번 참고해보면 좋을 것 같아요~!

suhongkim98 · 2024-03-06T10:38:01Z

+
+@Mapper(componentModel = "spring")
+public interface StudentMapper {
+    StudentMapper INSTANCE = Mappers.getMapper(StudentMapper.class);


INSTANCE를 선언했네요, 그럼 Mapstruct를 시제로 사용할 땐 빈으로 주입받지 않고 StudentMapper.INSTANCE.toDomain.. 꼴로 사용이 가능할 것 같아요.

feature : #4 개인정보 수집 구현

81080eb

Koonayoung518 requested a review from suhongkim98 February 18, 2024 15:23

suhongkim98 reviewed Feb 18, 2024

View reviewed changes

suhongkim98 assigned Koonayoung518 Feb 18, 2024

Koonayoung518 added 2 commits February 19, 2024 11:26

feature : #4 Consent 구조 변경, mapper에 Mapstruct 도입, 참인재 학생 정보 가져오기 구조 변경

8b35ac7

feature : #4 Consent 구조 변경, mapper에 Mapstruct 도입, 참인재 학생 정보 가져오기 구조 변경

f4a7e0a

suhongkim98 reviewed Feb 19, 2024

View reviewed changes

Koonayoung518 added 2 commits February 27, 2024 22:47

feature : #4 정보 수집 useCase 수정

999729c

feature : #4 정보 수집 동의 여부 조회 기능 구현

5940259

Koonayoung518 requested a review from suhongkim98 February 27, 2024 14:44

suhongkim98 requested changes Mar 6, 2024

View reviewed changes

feature : #4 정보 수집 동의 관련 기능 수정

a3c6c2b

Koonayoung518 requested a review from suhongkim98 April 18, 2024 09:40

Conversation

Koonayoung518 commented Feb 18, 2024

Uh oh!

suhongkim98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suhongkim98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

로직

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suhongkim98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants