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

3주차 미션 / 서버 4조 조동현 #1

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mr8356
Copy link

@mr8356 mr8356 commented Oct 2, 2024

이 미션 오늘 오후에 시작해서 벼락치기로 오늘 저녁에 끝냈는데, 이게 웬걸 1등이네요?ㅎㅎ

스크린샷 2024-10-02 오후 8 50 15

처음부터 끝까지 6시간 걸렸네요...(중간에 레이저 제모하러 가서 사실상 4시간이긴하지만) 파트장님 과제를 줄여주십쇼

userRepository를 계속 의존성 주입을 하면서 짜고 있는데, 코드가 계속 리팩토링되고 바뀌면서 DI적용하고, 또 변경된 구조에 DI 적용하고, 연속이니까 힘드네요!
처음부터 깔끔하게 작성하려고 노력하는 타입이라 리팩토링 연속으로 하면서 헛수고를 많이해서 말이 길어졌나 싶소

다들 홧팅!


코드 리뷰 잘 받았습니다!

특히 객체를 static으로 하면 싱글톤 패턴으로 관리할 수 있다는게 인상적이네요
싱글톤을 못지킬까봐 메인함수(당연히 1번만 실행된다는 보장이 있으므로)->handler-> mapper -> controller까지 받게했었던 겁니다.
mapper의 final static 필드로 Repository 인스턴스로 받아주고, n개의 컨트롤러에 DI 했습니다! 이렇게 하는게 맞겠죠? (스프링 컨테이너가 빈으로 관리하는게 익숙해져서 저도 헷갈리네요!)

수정해서 커밋해놨고 자세한 리뷰 정말 감사드립니다!

@mr8356 mr8356 requested review from hamhyeongju and JGoo99 and removed request for hamhyeongju October 2, 2024 11:48
Copy link
Member

@hamhyeongju hamhyeongju left a comment

Choose a reason for hiding this comment

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

코드 잘 봤습니다! 동현님께서 항상 미션을 열심히 수행해 주셔서 코드 리뷰 드리는 맛이 있네요! 코드가 보기 편해서 코드 리뷰도 수월했던 것 같습니다.
그나저나 pr에 ttttmi 가 포함돼있어서 좀 놀랐네요 ㅎ..ㅎ 미션 수행에 4시간 밖에 안걸리셨다니. 동현님껜 너무 미션이 쉬웠나 보네요. 미션이 시시하게 느끼셨을까봐 조금 걱정입니다.(동현님께는 특별히 추가 미션을 드리는 걸로...)
코드 리뷰 확인하시고 이따 추가로 올라갈 구현 코드 확인하시면서 부족한 부분(response test) 생각해보시면 좋을 것 같습니다! 수고 많으셨습니다!

src/main/java/webserver/controller/ForwardController.java Outdated Show resolved Hide resolved
src/main/java/webserver/controller/ListController.java Outdated Show resolved Hide resolved
src/main/java/webserver/controller/ListController.java Outdated Show resolved Hide resolved
String password = request.getBodyParams().get("password");
User user = userRepository.findUserById(userId);
if (user != null){
if (password.equals(user.getPassword())){
Copy link
Member

Choose a reason for hiding this comment

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

이 두 조건식은 user != null && password.equals(user.getPassword()로 작성하는게 가독성이 좋을 것 같지 않나요? 이 방식을 사용하건, 메서드 추출을 사용하건, 이중 들여쓰기를 의식적으로 제한하도록 노력해보세요!

Copy link
Author

Choose a reason for hiding this comment

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

급하게 작성하느라 놓쳤네요! 그러나 이건 논외의 애기지만, 제가 예전에 nodeJS, NestJS 공부할땐 아래와 같은 느낌으로 가독성 좋게 작성을 했었는데, 자바에서도 이렇게 작성하곤 하나요?

final boolean authenticated = user != null && password.equals(user.getPassword());
        if (authenticated){
            //todo 아직 쿠키 res에 대해선 리펙토링 안함.
            response.response302RedirectWithCookie(URLPath.INDEX.getPath(),"logined=true");
        }

src/main/java/webserver/controller/LoginController.java Outdated Show resolved Hide resolved
src/main/java/webserver/RequestHandler.java Outdated Show resolved Hide resolved
src/main/java/webserver/RequestHandler.java Outdated Show resolved Hide resolved
src/main/java/webserver/RequestHandler.java Show resolved Hide resolved
src/main/java/webserver/controller/LoginController.java Outdated Show resolved Hide resolved
src/main/java/webserver/RequestMapper.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants