[토니] 3단계 - HTTP 웹서버 구현 미션 제출합니다. #186
Open
Conversation
This file contains 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
- 가독성을 위한 상수화 - 메서드 순서 변경 - Login 관련 메서드 수정
phs1116
suggested changes
Nov 15, 2020
| String sessionId = UserService.login(request); | ||
|
|
||
| response.addHttpHeader(LOCATION, "/index.html"); | ||
| response.addHttpHeader(SET_COOKIE, "sessionId=" + sessionId + "; " + PATH); |
There was a problem hiding this comment.
Response 내에 Session이라는 객체를 한번 추상화해보고, 요청 혹은 응답 메시지를 만들 때 쿠키에 세션아이디를 만들어 저장하도록 해보면 어떨까요?
Comment on lines
+50
to
+51
| String sessionId = SessionStorage.create(USER_ID, user.getUserId()); | ||
| SessionStorage.loginBy(sessionId); |
There was a problem hiding this comment.
표현계층에 가까운 세션 스토리지를 서비스 영역에서 만드는건 역할에 맞지 않은 것 같아요.
|
|
||
| public static String create(String userId, Object value) { | ||
| HttpSession httpSession = new HttpSession(); | ||
| httpSession.setAttributes(userId, value); |
There was a problem hiding this comment.
httpServletRequest, httpServletResponse를 참고해서 세션을 어떤식으로 가져오고 처리하는지 살펴보면 좋을 것 같아요!
개발자가 스토리지를 통해 직접 저장하는것보다는, 세션만 관리하고 프레임워크가 자동으로 저장하도록 추상화 방향을 잡아보면 어떨까요?
Comment on lines
+23
to
+26
| public static void loginBy(String sessionId) { | ||
| HttpSession httpSession = sessions.get(sessionId); | ||
| httpSession.setAttributes("logined", true); | ||
| } |
There was a problem hiding this comment.
loginBy, create는 세션 스토리지라는 이름과 걸맞지 않게 너무 비즈니스 요소가 많이 들어가있어요.
두개를 분리해서 생각하도록 고민해보면 좋을 것 같아요.
| return getSessionId(request); | ||
| } | ||
|
|
||
| String sessionId = SessionStorage.create(USER_ID, user.getUserId()); |
There was a problem hiding this comment.
서비스 영역에서 응답으로 세션 ID를 전달받고, 이를 표현영역에서 쿠키에 직접 세션키를 세팅하고 있는데요,
사실 세션이라는게 사용자 요청에 의해 세션키가 생성이 되는거라 굳이 이럴 필요가 없습니다.
이 부분에서도 추상화를 하여 요청과 응답을 만들 때 세팅이 되도록 해보면 어떨까요?
Comment on lines
+15
to
+48
| private static final String USER_ID = "userId"; | ||
| private static final String SESSION_ID = "sessionId="; | ||
| private static final String PASSWORD = "password"; | ||
| private static final int SESSION_ID_KEY_LENGTH = 10; | ||
| private static final int SESSION_ID_LENGTH = 46; | ||
|
|
||
| public static void create(HttpRequest request) { | ||
| User user = new User( | ||
| request.getHttpBodyValueOf(USER_ID), | ||
| request.getHttpBodyValueOf(PASSWORD), | ||
| request.getHttpBodyValueOf("name"), | ||
| request.getHttpBodyValueOf("email") | ||
| ); | ||
| DataBase.addUser(user); | ||
| } | ||
|
|
||
| public static List<User> findAll() { | ||
| return Collections.unmodifiableList(DataBase.findAll()); | ||
| } | ||
|
|
||
| public static String login(HttpRequest request) { | ||
| User user = DataBase.findUserById(request.getHttpBodyValueOf(USER_ID)); | ||
|
|
||
| if (user == null) { | ||
| throw new UserLoginException("해당되는 id의 사용자가 없습니다."); | ||
| } | ||
|
|
||
| if (!Objects.equals(request.getHttpBodyValueOf(PASSWORD), user.getPassword())) { | ||
| throw new UserLoginException("올바르지 않은 비밀번호입니다."); | ||
| } | ||
|
|
||
| if (isLogin(request)) { | ||
| return getSessionId(request); | ||
| } |
There was a problem hiding this comment.
표현계층에 대한 정보(요청)등은 표현에서만 책임지는게 좋을 것 같네요. :)
User등의 정보를 만들어서 전달하도록 해보면 어떨까요?
Comment on lines
+64
to
+68
| private static String getSessionId(HttpRequest request) { | ||
| String cookie = request.getHttpHeaderParameterOf("Cookie"); | ||
| return cookie.substring(cookie.indexOf(SESSION_ID) + SESSION_ID_KEY_LENGTH, | ||
| cookie.indexOf(SESSION_ID) + SESSION_ID_LENGTH); | ||
| } |
Comment on lines
+56
to
+60
| if (request.hasHttpHeaderParameterOf("Cookie")) { | ||
| String sessionId = getSessionId(request); | ||
| HttpSession httpSession = SessionStorage.findBy(sessionId); | ||
| return (boolean)httpSession.getAttribute("logined"); | ||
| } |
| logger.debug("error : {}", e.getMessage()); | ||
|
|
||
| response.addHttpHeader(LOCATION, "/user/login_failed.html"); | ||
| response.addHttpHeader(SET_COOKIE, "logined=false; " + PATH); |
There was a problem hiding this comment.
헤더에 직접 쿠키를 세팅하지말고, 쿠키 또한 추상화를 해보면 어떨까요?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
안녕하세요 화투 🙂
많이 늦었지만 돌아왔습니다~!
부족한 부분이 많습니다. 가감없이 리뷰 부탁드려요 😌
항상 감사합니다!