Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e6e490d
2단계 리팩토링 가이드 추가
Mar 27, 2021
c1bcc8d
Merge pull request #19 from janeljs/readme.md
janeljs Mar 27, 2021
074a52e
refactor: 테스트 코드 레거시 수정
janeljs Mar 29, 2021
c5a9085
test: Request 메시지 테스트 작성
janeljs Mar 29, 2021
732de5b
fix: GET일 경우 쿼리 스트링에서, POST일 경우 리퀘스트 바디에서 파라미터 추출
janeljs Mar 29, 2021
65f8d64
test: HttpResponse 클래스 테스트 코드 작성
janeljs Mar 29, 2021
47bcfce
Refactor: Controller 패키지를 추가하여 중복 코드 제거
janeljs Mar 29, 2021
2ef2e76
Refactor: URL에 대한 분기 처리 제거 및 response200Header 메서드 리팩토링
janeljs Mar 29, 2021
b7eb9c3
Merge pull request #21 from janeljs/refactor2
Mar 29, 2021
2dfd22b
fix : POST에서도 queryString을 활용한 데이터 전달 지원
Mar 30, 2021
7f1068b
refactor : HttpRequest, HttpResponse 리팩토링
Mar 30, 2021
5a8b133
Merge pull request #23 from janeljs/refactor2
janeljs Mar 30, 2021
950d599
refactor: 기능 단위로 메서드 분리
janeljs Mar 31, 2021
21d952a
fix: 쿠키 보안의 취약점을 개선하기 위해 세션 DB 추가
janeljs Mar 31, 2021
c75f752
Merge pull request #24 from janeljs/refactor3
Mar 31, 2021
82b8cda
refactor : 전체 리팩토링
Mar 31, 2021
56be465
refactor: SoftAssertionsExtension을 활용하여 테스트 코드 리팩토링
janeljs Mar 31, 2021
cec3f61
Merge pull request #25 from janeljs/refactor4
Mar 31, 2021
9084520
refactor: 쓰레드풀을 사용하도록 리팩토링
janeljs Apr 2, 2021
10b36ff
Merge pull request #27 from janeljs/refactor4
Apr 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@
### 요구사항 7
- 지금까지 구현한 소스 코드는 stylesheet 파일을 지원하지 못하고 있다. Stylesheet 파일을 지원하도록 구현하도록 한다.

## 2단계 - 리팩토링




- 리팩토링 접근 방법 - 메소드 분리 및 클래스 분리
- 클라이언트 요청 데이터를 처리하는 로직을 별도의 클래스로 분리한다.(HttpRequest)
- 클라이언트 응답 데이터를 처리하는 로직을 별도의 클래스로 분리한다.(HttpResponse)
- 다형성을 활용해 클라이언트 요청 URL에 대한 분기 처리를 제거한다.
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ dependencies {
implementation 'org.apache.commons:commons-dbcp2:2.5.0'
implementation 'org.reflections:reflections:0.9.11'
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.6.0'
testImplementation 'org.assertj:assertj-core:3.11.1'
testImplementation 'org.assertj:assertj-core:3.18.0'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine'
runtimeOnly 'com.h2database:h2:1.4.200'
}

test {
useJUnitPlatform()
}
}
27 changes: 27 additions & 0 deletions src/main/java/controller/BaseController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package controller;

import http.HttpMethod;
import http.HttpRequest;
import http.HttpResponse;

public class BaseController implements Controller {

@Override
public void service(HttpRequest request, HttpResponse response) {
HttpMethod method = request.getMethod();

if (method.isPost()) {
doPost(request, response);
} else {
doGet(request, response);
}
Comment on lines +13 to +17
Copy link
Member

Choose a reason for hiding this comment

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

if else를 제거할 수 있는 방법은 없을까요?

}

public void doPost(HttpRequest request, HttpResponse response) {

}

public void doGet(HttpRequest request, HttpResponse response) {

}
Comment on lines +20 to +26
Copy link
Member

Choose a reason for hiding this comment

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

아직 리팩토링 중이어서 구현체가 없는걸까요?

Copy link
Member

Choose a reason for hiding this comment

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

만약에 오버라이딩 하기 위함이라면
abstract가 더 적절하지 않았을까 싶네요~

}
8 changes: 8 additions & 0 deletions src/main/java/controller/Controller.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package controller;

import http.HttpRequest;
import http.HttpResponse;

public interface Controller {
Copy link
Member

Choose a reason for hiding this comment

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

인터페이스를 사용해주셨네요 👍

void service(HttpRequest request, HttpResponse response);
}
28 changes: 28 additions & 0 deletions src/main/java/controller/CreateUserController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package controller;

import db.DataBase;
import model.User;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import http.HttpRequest;
import http.HttpResponse;

public class CreateUserController extends BaseController {
private static final Logger log = LoggerFactory.getLogger(CreateUserController.class);

@Override
public void doPost(HttpRequest request, HttpResponse response) {
User user = getUserFrom(request);
DataBase.addUser(user);
response.sendRedirect("/index.html");
Copy link
Member

Choose a reason for hiding this comment

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

상수로 적절히 추출해봅시다!

log.debug("{}님이 회원가입에 성공하셨습니다.", user.getUserId());
}

private User getUserFrom(HttpRequest request) {
String userId = request.getParameter("userId");
String password = request.getParameter("password");
String name = request.getParameter("name");
String email = request.getParameter("email");
return new User(userId, password, name, email);
}
Comment on lines +21 to +27
Copy link
Member

Choose a reason for hiding this comment

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

스프링은 어떻게 이걸 처리해줄까요? 한 번 공부해봅시다.

}
74 changes: 74 additions & 0 deletions src/main/java/controller/ListUserController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package controller;

import db.DataBase;
import db.SessionDataBase;
import model.User;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import util.HttpRequestUtils;
import http.HttpRequest;
import http.HttpResponse;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.Collection;
import java.util.Map;

public class ListUserController extends BaseController {
Copy link
Member

Choose a reason for hiding this comment

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

우리가 만들었던 스프링 Application은 한 Controller에서 해당 역할과 연관된 일을 전부 수행해줄 수 있었습니다.
이는 어떻게 가능한걸까요? BaseController가 일일이 get post를 분리해서 처리하고 있는데, 더 좋은 방법이 없는걸까요?

Copy link
Member

Choose a reason for hiding this comment

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

만약 HTTP Method들이 하나씩 늘어나면, 분기문은 계속해서 추가될텐데, 그 때 마다 기본 동작을 일일이 재정의 해줘야하는 걸까요?

private static final Logger log = LoggerFactory.getLogger(ListUserController.class);

@Override
public void doGet(HttpRequest request, HttpResponse response) {
String sessionId = request.getHeader("Cookie");
if (!isLogin(sessionId)) {
response.sendRedirect("/user/login.html");
} else {
String body = getBody("/user/list.html");
int tbodyIndex = body.indexOf("<tbody>");
String result = addUserList(body, tbodyIndex).toString();
response.forwardBody(result);
Comment on lines +27 to +30
Copy link
Member

Choose a reason for hiding this comment

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

이건 View가 해줘야 할 책임이 아닐까요?

}
Comment on lines +23 to +31
Copy link
Member

Choose a reason for hiding this comment

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

여기서도 if else 문이 사용되었네요. 최대한 지양해봅시다.
void도 early return을 사용해 줄 수 있습니다.

}

private boolean isLogin(String id) {
Map<String, String> cookieStringMap = HttpRequestUtils.parseCookies(id);
log.info("Cookie: {}", cookieStringMap.toString());
return SessionDataBase.isLoginUser(cookieStringMap.get("JSESSIONID"));
Copy link
Member

Choose a reason for hiding this comment

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

하드 코딩된 부분은 상수로 추출해서 사용해봅시다.
Session 관리하는 부분은 좋네요 👍

}

private String getBody(String url) {
byte[] body = new byte[0];
try {
body = Files.readAllBytes(new File("./webapp" + url).toPath());
} catch (IOException e) {
e.printStackTrace();
}
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

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

이 부분의 예외처리를 이렇게 해주신 이유가 있나요?
만약 IOException이 발생했는데 이렇게 된다면 어떻게 처리해주는게 적절할까요?

대략적으로 생각해보면, 요청한 파일이 없는 경우가 되지 않을까요?

return new String(body);
}

private StringBuilder processUserList(String body, int tbodyIndex) {
StringBuilder result = new StringBuilder(body.substring(0, tbodyIndex + 7));
Collection<User> users = DataBase.findAll();
int id = 0;
for (User user : users) {
id++;
result.append("<tr><th scope=\"row\">")
.append(id)
.append("</th> <td>")
.append(user.getUserId())
.append("</td> <td>")
.append(user.getName())
.append("</td> <td>")
.append(user.getEmail())
.append("</td> <td>")
.append("<a href=\"#\" class=\"btn btn - success\" role=\"button\">수정</a></td>");
}
return result;
Comment on lines +51 to +67
Copy link
Member

Choose a reason for hiding this comment

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

요것도 역시 Controller가 가질 부분은 아니죠~
그리고, DB까지 View에서 확인하고 있습니다. 개선 부탁드려요~

}

private StringBuilder addUserList(String body, int tbodyIndex) {
StringBuilder result = processUserList(body, tbodyIndex);
return result.append(body.substring(tbodyIndex + 7));
}
Comment on lines +34 to +73
Copy link
Member

Choose a reason for hiding this comment

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

private 메서드의 사용이 많습니다.
많아야 했나요? 더 좋은 방법은 없을까요?

}
32 changes: 32 additions & 0 deletions src/main/java/controller/LoginController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package controller;

import db.DataBase;
import db.SessionDataBase;
import model.User;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import http.HttpRequest;
import http.HttpResponse;

import java.util.UUID;

public class LoginController extends BaseController {
private static final Logger log = LoggerFactory.getLogger(LoginController.class);

@Override
public void doPost(HttpRequest request, HttpResponse response) {
String userId = request.getParameter("userId");
String password = request.getParameter("password");
User targetUser = DataBase.findUserById(userId);
Copy link
Member

Choose a reason for hiding this comment

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

우리가 Spring을 사용하면서 써왔던 패턴 대로 개발해봅시다.
DB를 여기서 접근하는게 맞을까요?

if (targetUser == null || !targetUser.isValidPassword(password)) {
response.addHeader("Set-Cookie", "loggedIn=false");
Copy link
Member

Choose a reason for hiding this comment

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

로그인 상태를 유저가 관리하는 형태인데 이게 안전할까요?

response.sendRedirect("/user/login_failed.html");
return;
}
UUID uuid = UUID.randomUUID();
SessionDataBase.sessions.put(uuid.toString(), targetUser);
Copy link
Member

Choose a reason for hiding this comment

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

SessionDataBase에 직접 접근하기보다는 별도의 클래스를 만들어서 거치도록 해줍시다.

response.addHeader("Set-Cookie", SessionDataBase.JSESSIONID + "=" + uuid);
Copy link
Member

Choose a reason for hiding this comment

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

Set-Cookie도 상수로 빼고, =도 상수로 빼줄 수 있을 것 같네요~

response.sendRedirect("/index.html");
log.info("{}님이 로그인하셨습니다.", userId);
}
}
20 changes: 20 additions & 0 deletions src/main/java/db/SessionDataBase.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package db;

import com.google.common.collect.Maps;
import model.User;

import java.util.Map;

public class SessionDataBase {
public static Map<String, User> sessions = Maps.newHashMap();
Copy link
Member

Choose a reason for hiding this comment

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

외부에서 접근해도 되는 걸까요?
심지어 final이 아니라서 변경까지 가능할 것 같네요.

Maps.newHashMap();은 기본제공되는게 아닌데, 혹시 어떤 이유에의해서 사용해주셨나요?


public static final String JSESSIONID = "JSESSIONID";
Copy link
Member

Choose a reason for hiding this comment

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

👍


public static boolean isLoginUser(String sessionId) {
return getSessionUser(sessionId) != null;
}
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

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

이곳에는 DB와 관련된 로직만 넣어줍시다.


public static User getSessionUser(String sessionId) {
return sessions.get(sessionId);
}
}
9 changes: 9 additions & 0 deletions src/main/java/http/HttpMethod.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package http;

public enum HttpMethod {
GET, POST;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GET, POST;
GET,
POST;


public boolean isPost() {
return this == POST;
}
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

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

다른 메서드는 정의될 필요가 없나요?

}
85 changes: 85 additions & 0 deletions src/main/java/http/HttpRequest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package http;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import util.HttpRequestUtils;
import util.IOUtils;

import java.io.*;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;

public class HttpRequest {
private static final Logger log = LoggerFactory.getLogger(HttpRequest.class);

private String requestLine;
private final Map<String, String> headers = new HashMap<>();
private Map<String, String> parameters;
Comment on lines +17 to +18
Copy link
Member

Choose a reason for hiding this comment

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

일급콜렉션에 대해서 학습해보시고 적용해보시는것도 좋을 것 같네요.


public HttpRequest(InputStream in) {
try {
BufferedReader br = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8));
requestLine = br.readLine();
processHeaders(br);
processParameters(br);
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

값을 리턴받아 할당하는 형태로 되는건 어떤가요?

Copy link
Member

Choose a reason for hiding this comment

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

그리고 생성자가 하는 일이 너무 많은 것 같아요!

} catch (IOException e) {
e.printStackTrace();
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

예외처리를 좀 더 신경써서 해주세요!

}
}

private void processHeaders(BufferedReader br) throws IOException {
String line = br.readLine();
while (!line.isEmpty()) {
log.debug("headers : {}", line);
line = br.readLine();
if (line == null) {
Copy link
Member

Choose a reason for hiding this comment

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

이 조건문이 여기 나와야할까요?

return;
}
parseHeader(line);
}
}

private void parseHeader(String line) {
String[] headerTokens = line.split(": ");
if (headerTokens.length == 2) {
headers.put(headerTokens[0], headerTokens[1]);
}
Comment on lines +44 to +47
Copy link
Member

Choose a reason for hiding this comment

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

리터럴들을 상수로 추출해주면 더 읽기 좋은 코드가 될 것 같네요~

}

private void processParameters(BufferedReader br) throws IOException {
String[] tokens = requestLine.split(" ");
String queryString = "";
String requestBody = "";
if (tokens[1].contains("?")) {
Copy link
Member

Choose a reason for hiding this comment

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

적절하게 tokens[1]을 변수로 추출해보세요~.

queryString = tokens[1].substring(tokens[1].indexOf("?") + 1);
}
if (headers.get("Content-Length") != null) {
requestBody = IOUtils.readData(br, Integer.parseInt(headers.get("Content-Length")));
}
queryString = queryString + "&" + requestBody;
parameters = HttpRequestUtils.parseQueryString(queryString);
}

public HttpMethod getMethod() {
String[] tokens = requestLine.split(" ");
String method = tokens[0];
return HttpMethod.valueOf(method);
}

public String getPath() {
String[] tokens = requestLine.split(" ");
if (tokens[1].contains("?")) {
return tokens[1].substring(0, tokens[1].indexOf("?"));
}
return tokens[1];
}
Comment on lines +70 to +76
Copy link
Member

Choose a reason for hiding this comment

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

tokens[1]이 무엇인지~ 역시 변수로 추출해봅시다.


public String getHeader(String fieldName) {
return headers.get(fieldName);
}

public String getParameter(String fieldName) {
return parameters.get(fieldName);
}
Comment on lines +78 to +84
Copy link
Member

Choose a reason for hiding this comment

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

좋네요~

}
Loading