feature : #3 Knu 로그인 Api 이관#10
Open
kymokim wants to merge 3 commits into
Open
Conversation
…application.yml 변경
suhongkim98
reviewed
Feb 26, 2024
Contributor
suhongkim98
left a comment
suhongkim98
left a comment
There was a problem hiding this comment.
마찬가지로 큰 틀만 일단 리뷰해드렸어요~~
suhongkim98
reviewed
Mar 6, 2024
Contributor
suhongkim98
left a comment
suhongkim98
left a comment
There was a problem hiding this comment.
굳굳 수정 후 훨씬 괜찮아진 것 같아요~~!
| throw new LoginFailedException(); // 사번이 1로 시작하는 교직원은 이용불가 | ||
| } | ||
|
|
||
| Map<String, Object> responseList = loginKnuUseCase.loginKnu(loginDto.getId(), loginDto.getPassword()); |
Contributor
There was a problem hiding this comment.
유즈케이스가 Map<String, Object> 를 리턴하도록 되어있는데 만약 map이 아닌 특정 필드를 추가해야하는 경우, 컨트롤러도 수정해주는 일이 생길 것 같아요.
dto를 리턴하도록 하고 컨트롤러에서는 기존 하위호환성 맞추기 위해 map을 꺼내 리턴하도록 하는게 어떨까요?
Contributor
Author
There was a problem hiding this comment.
LoginKnuCommand 사용하여 리턴하도록 개선했습니다 !
| public class LoginKnuService implements LoginKnuUseCase { | ||
|
|
||
| private final GetSessionInfoPort getSessionInfoPort; | ||
| private final GetStudentInfoPort getStudentInfoPort; |
Contributor
There was a problem hiding this comment.
나영이랑 상의해서 포트 컨벤션 맞추는게 좋을 것 같아요.
근데 get은 워낙에 이곳 저곳에서 많이 쓰이는 키워드라서 load..도 좋아보입니다..!
| responseList.put("studentInfo", studentInfo); | ||
|
|
||
| return responseList; | ||
| } |
| private final ApiEndpointSecretProperties apiEndpointSecretProperties; | ||
|
|
||
| @Override | ||
| public Map<String, Object> getSessionInfo(Map<String, String> data){ |
Contributor
There was a problem hiding this comment.
합친 것은 되게 좋았는데 하나로 합치는 과정에서 getSessionInfo 메서드의 책임이 너무 많아진 것 같아용
적절한 책임을 private 메서드로 따로 빼고 getSessionInfo메서드에서는 프라이빗 메서드를 호출하는 형태로 바꾸는게 어떨까요?
Contributor
Author
There was a problem hiding this comment.
각각의 load 함수 3개로 분리하였고, 공통적으로 사용되는 Jsoup.connect 함수도 모듈화했습니다.
| @@ -0,0 +1,4 @@ | |||
| package com.example.allknuauth.auth.domain; | |||
|
|
|||
| public class Auth { | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
knu 로그인 api를 이관하였습니다.
정상 작동 확인은 되었으나 올바른 구조는 아닌 듯 하여 코멘트 및 #4 를 참고하여 수정, 보완하도록 하겠습니다 !