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

[ToDo REST API 만들기] 1주차 과제 제출 #42

Merged
merged 54 commits into from
Aug 16, 2021

Conversation

jxmen
Copy link

@jxmen jxmen commented Aug 9, 2021

우선 오늘까지 작업한 부분 제출합니다!

  • ToDo 목록 얻기 - GET /tasks
  • ToDo 상세 조회하기 - GET /tasks/{id}
    • taskID 를 못찾을시 404 에러
  • ToDo 생성하기 - POST /tasks
  • ToDo 제목 수정하기 - PUT/PATCH /tasks/{id}
  • ToDo 삭제하기 - DELETE /tasks/{id}

일단 아직 더 해야될 것 같은걸로는.. HttpResponse를 따로 분리하여 404에러 코드 부분을 중복 제거를 하면 좋을 것 같습니다.
리팩토링이 더 많이 필요할 것 같네요. 😒

jxmen added 11 commits August 9, 2021 15:54
GET /tasks -> 할일 목록 전체 얻기
GET /tasks/{id} - 할일 상세 조회
taskId와 content를 받아오는 부분 메서드로 리팩토링
GET /tasks/{id} : 기존 title 응답
POST /tasks: Hello, World! 응답

-> 모두 할일 JSON을 응답하도록 수정
PUT /tasks/{id}
PATCH /tasks/{id}
DELETE /tasks/{id}
http response를 보내는 과정을 함수로 분리
content = tasksToJson();
}

boolean isNumberMatchEndOfPath = Pattern.matches("/tasks/[0-9]+$", path);
Copy link
Contributor

Choose a reason for hiding this comment

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

변수의 이름을 그대로 읽어보면 path의 마지막에 숫자가 존재하지는지 확인하는 것 같습니다.

이 변수가 무엇을 하려고 작성하신건지는 이해했습니다. 하지만 하는지는 드러나지 않는 것 같습니다.

만약 이렇게 작성해보면 어떨까요?

boolean hasTaskId = Pattern.matches("/tasks/[0-9]+$", path);

제가 생각하기에는 할 일 목록중 특정한 아이디가 존재하는지 찾기 위해 패스 파라미터로 아이디를 같이 넘겨주었는지 확인하고 싶으셨던 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요. 변수명을 지을 때 왜? 인지를 더 고민할 수 있을 것 같습니다.
무엇을 할려고 하는 것보다는 본질적으로 이것을 왜 하려고 하는지가 더 중요할 것 같네요

  • 무엇: path 끝에 number가 존재하는가?
  • 왜: number를 왜 쓸려고 하는가? - TaskId가 존재하는지 확인하기 위해

Comment on lines 130 to 131
Long newTaskId = ((long) (tasks.size() + 1));
task.setId(newTaskId);
Copy link
Contributor

Choose a reason for hiding this comment

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

새로운 아이디를 만들 때, 현재 데이터의 길이보다 하나 더 큰 값으로 아이디를 만들고 있는 것 같습니다. 이럴경우 다음과 같은 상황에서 문제가 발생할 수 있습니다.

1
2
3

이러한 데이터가 있을 때 중간 값을 삭제합니다.

1
3

이 상황에서 새로운 값을 하나 추가합니다. 길이(2) + 1을 더한 값을 아이디로 할당하니 다음과 같은 결과가 나옵니다.

1
3
3

따라서 아이디를 만들 떄는 데이터의 길이를 가지고 만들어서는 안됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

lastTaskId 라는 인스턴스 변수를 만들고 새로운 task가 추가될 때 이 값이 증가되도록 하였습니다.

Comment on lines 136 to 141
private Task getTaskFromId(long taskId) {
return tasks.stream()
.filter(task -> task.isMatchId(taskId))
.findFirst()
.orElse(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

함수가 null을 반환할지 안할지는 함수의 시그니처(리턴 타입, 매개변수, 함수의 이름)만 봐서는 절대 알 수 없습니다.
그래서 사용하는 입장에서 반환값이 항상 null인지 확인해야하고, 만약 이를 까먹을경우 NullPointerException이 발생할 수 있습니다.

따라서 함수의 반환값으로 null을 반환해서는 안됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요. 제가 이 글을 보고나서, isPresent()-get() 방식을 쓰지 않으려다 보니 이런식으로 작성한 것 같습니다.

그래서 제 생각에는 Optional을 반환하고 이를 다시 꺼내는 것보다는, 아직 구현하지는 않았지만 orElse() 에서 404 응답을 주는 것도 괜찮은 방법 같네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

그러셨군요 ㅎㅎ 저는 null을 사용하는 것보다는 Optional을 반환하는게 더 좋다고 생각합니다. 왜냐하면 해당 함수를 사용하는 사람은 무조건 없는경우를 처리하도록 강요할 수 있기 때문이에요. 함수의 시그니처만 보아도 없을 수 있다는 것을 알 수 있죠.

그런데 무조건 Optional을 반환해야 하는 것도 아니긴 합니다. ㅎㅎ 우리에겐 예외를 처리하는 방법이 있으니까요

Comment on lines 33 to 39
@Override
public String toString() {
return "Task{" +
"id=" + id +
", title='" + title + '\'' +
'}';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

String.format을 사용하는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

동의합니다. intellij에서 toString()을 만들 때 있는 것을 그대로 사용하였는데, String.format 이 더 깔끔할 것 같네요.

@jxmen
Copy link
Author

jxmen commented Aug 10, 2021

08.10 작업내용

  • 코드리뷰 내용 반영
  • 요청과 응답 HttpRequest, HttpResponse 등으로 분리
  • Task의 리스트를 관리하는 TaskManager, Task의 데이터를 Json으로 매핑하는 TaskMapper등으로 추출

마감시간 때문에, 리팩토링 진행중에 푸쉬를 하여 테스트가 모두 통과되지 않았네요.. 하지만 중간 작업을 빠르게 공유하는 것이 좋다고 판단되어 우선 작업중인 내역 올립니다! 이점 감안해서 리뷰 부탁드립니다!

Comment on lines 39 to 40
task.setId(++lastTaskId);
tasks.add(task);
Copy link
Contributor

Choose a reason for hiding this comment

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

lastTaskId++;
task.setId(lastTaskId);
tasks.add(task);

이렇게 쓰는게 더 이해하기 쉬울 것 같아요

Copy link
Author

@jxmen jxmen Aug 11, 2021

Choose a reason for hiding this comment

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

마지막 taskId를 증가시키는 로직을 분리한 거군요.
해당 사항 적용했고, tasks는 이미 taskManager에서 관리하고 있어서, lastId 라는 이름으로 변경하였습니다.

Comment on lines 45 to 50
public Task deleteTask(long taskId) {
Task task = findTaskFromId(taskId);
tasks.remove(task);

return task;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

public Task deleteTask(long id) {
    Task task = findTask(id);
    tasks.remove(task);

    return task;
}

findTaskFromId이 id에서 Task를 찾는다라고 읽히는 것 같아요. findTask라고만 해도 task를 찾는다는 것을 드러낼 수 있고, 파라미터로 id를 넘겨주고 있으니 id라는 것도 드러낼 수 있어서 findTask라고만 써줘도 괜찮을 것 같아요.

만약 id로 찾는 것이 아니라 나중에 다른 수단으로 찾는 것이 생긴다면 findTaksWith같은 이름으로 쓰면 되지 않을까? 생각이 들었습니다

Copy link
Author

Choose a reason for hiding this comment

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

findTaskWith 라는 이름이 괜찮네요! 네이밍을 할 때 파라미터도 고려해볼 수 있겠네요.

import java.io.OutputStream;
import java.util.List;

public class TaskMapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Task를 받아서 JSON문자열로 반환해주는 객체로 이해했는데, TaskManager에서 직접적으로 할 일 목록도 가져와서 반환해주고 있는 것 같아요. 이러한 의존성을 밖에서 드러나지 않으므로, 객체들간의 의존성을 파악하기 어려워져 결론적으로 유지보수하기 힘든 코드가 됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

제가 정확히 이해한 것인지는 모르겠는데, 해당 의존성을 드러내기 위해 TaskManager를 인스턴스 변수 등으로 만들어야 한다는 것인가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

TaskMapper가 에 만약 변경이 일어난다면 어떤 것들 때문일까요? 먼저 Task가 변경된다면 TaskMapper에 변경이 일어날 수 있겠죠. 그렇다는 것은 TaskMapper는 Task에 의존하고 있다는 거에요. 그리고 TaskManager가 변경될때도 TaskMapper에 변경이 일어나겠죠.

근데 이게 TaskManager를 사용할 때는 이러한 의존성이 있는지 알 수 없습니다. 그래서 무언가를 변경했을 때 어떤 영향이 있는지 가늠하기가 굉장히 어려워집니다.

따라서 객체가 서로 의존할 때는 어떻게 의존 관계를 가질지 고민해야 합니다.

.replace("/", "");

return Long.parseLong(replaced);
}

public boolean isReadAll() {
return "GET".equals(method) && "/tasks".equals(path);
return "GET".equals(method) && PREFIX_PATH.equals(path);
Copy link
Author

Choose a reason for hiding this comment

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

"GET".eqauls(method) 이부분도 enum으로 비교하면 좋을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

열거 타입을 사용했을 때 어떤 것이 더 좋은가요? 이펙티브 자바의 아이템 34를 확인해보면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

제가 원래 알고 있던 열거형의 장점으로는, 연관된 상수들을 모아서 관리하는 것이였습니다.
책을 읽어서 추가로 알게된 점은, 각 상수와 연관된 로직 등을 메서드에 넣을 수 있는 점, 기존의 열거형을 수정해도 재컴파일 하지 않아도 되는 장점이 있는 것 같습니다.

책의 코드를 어느정도 참고했으나, 앞으로 enum에 대해 더 공부해야 될 것 같네요😭

return String.format("HttpRequest {method=%s, path=%s} ", method, path);
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

코드 마지막 라인에 빈 줄 하나를 추가해주세요

Copy link
Author

@jxmen jxmen Aug 12, 2021

Choose a reason for hiding this comment

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

현재 ideaVim (vim) 플러그인을 사용중인데, 노멀모드에서 :w로 저장할때는 파일 마지막에 공백이 생기는데, command+s 로 저장할때는 공백이 생기지 않더군요. 그래서 keymap을 찾아보았는데 예전에 설정한 포매터 매크로가 파일 마지막에 공백을 추가하지는 않는 것 같네요

image

그래서 이제 해당 매크로를 지우니까 command+s로 저장해도 공백이 다시 생기네요ㅎ

Comment on lines 44 to 48
try {
taskId = httpRequest.getTaskIdFromPath();
} catch (NumberFormatException ne) {
taskId = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 사용하면 null을 반환하는 것과 크게 다르지 않은 것 같아요. null을 사용하는 순간 이 아래부터 이 taskIdnull인지 아닌지를 항상 인지하고 있어야 합니다

Copy link
Author

Choose a reason for hiding this comment

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

함수 내에서 path 내에 taskId 가 있는지 검사하여 비어있으면 EMPTY_TASK_ID로 0을 주도록 수정하였습니다.

Comment on lines 72 to 90
if (httpRequest.isUpdateOne()) {
try {
Task updatedTask = taskManager.updateTask(taskId, body);

new HttpResponseOK(httpExchange).send(taskMapper.toJsonWith(updatedTask));
} catch (TaskIdNotFoundException error) {
new HttpResponseNotFound(httpExchange).send(error.getMessage());
}
}

if (httpRequest.isDeleteOne()) {
try {
Task deletedTask = taskManager.deleteTask(taskId);

new HttpResponseNoContent(httpExchange).send(taskMapper.toJsonWith(deletedTask));
} catch (TaskIdNotFoundException error) {
new HttpResponseNotFound(httpExchange).send(error.getMessage());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

굉장히 깔끔하게 잘 처리해주신 것 같습니다 ㅎㅎ 낄끔하게 정리되느 무언가 비슷한 패턴이 보이는 군요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

지금 패턴이

  1. taskID를 가져온다.
  2. httpRequest에 따라 작업을 수행하고 그에 맞는 Response를 보낸다. (create, update, delete 등등..)
  3. 예외가 날 경우 그에 따른 4xx Response를 보낸다.

이 3가지 형태로 가는 것 같네요. 극단적으로는 If문 까지도 제거해 볼수도 있을 것 같은데, 지금은 딱히 좋은 인사이트가 떠오르지는 않네요. 😒

Copy link
Contributor

Choose a reason for hiding this comment

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

오 ㅎㅎ 각 문제를 굉장히 잘 분석하신 것 같습니다. 각 일에 대해서 누가 그 일을 하면 가장 적합할지 고민해보면 좋을 것 같습니다

@jxmen
Copy link
Author

jxmen commented Aug 12, 2021

08.12

리뷰 내용 반영

  • HttpRequest method를 AllowedMethods라는 열거형으로 포장
  • httpRequest path에서 taskId를 찾아 반환하는 메서드에서, taskId가 없을 경우 null 대신 0 반환

그 외

  • TaskHttpHandler - 몇몇 작업 private 메서드로 분리

질문

  1. 커밋 메시지가 refactor가 너무 많은 것 같네요ㅎㅎ 어떤 작업을 메서드나 클래스로 추출하는 것도 refactor로 하는 게 좋을까요?
  2. 현재는 TaskManager가 너무 많은 일을 하는 것 같습니다. TaskHttpHandler에서 변경 요청 (create, update, delete) 요청도 받고, TaskMapper에서 값을 읽는 작업을 하고 있는데, 이를 TaskReader, TaskChanger 등의 두 클래스로 분리하는건 어떨까요?

Comment on lines 7 to 14
public class TaskConvertor {

private final ObjectMapper objectMapper = new ObjectMapper();

public Task toTask(String content) throws JsonProcessingException {
return objectMapper.readValue(content, Task.class);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

문자열을 받아서 객체를 만드는역할을 하는거군요 ㅎㅎ 팩토리라고 불러도 괜찮을까요?

Copy link
Author

Choose a reason for hiding this comment

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

저는 변환한다라는 것에 초첨을 두어 TaskConvertor라는 이름을 지었는데, 객체를 만든다 라고 생각하면 factory도 괜찮은 이름 같네요

Copy link
Contributor

Choose a reason for hiding this comment

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

이름 뿐만 아니라 객체의 생성의 책임을 가지고 있는 팩토리도 생각이나서 말씀드려봤습니다

return tasks;
}

public Task createTask(String body) throws JsonProcessingException {
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP 요청에서 body의 데이터를 가지고 Task를 만들다보니 body라는 변수 이름을 사용하신 것 같습니다.
이것은 여기 코드에서 필요하지 않은 컨텍스트(문맥)이 침범한 것으로, 유지보수하기 어려운 코드가 될 가능성이 높습니다.
따라서 body대신에 title이라고 하는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

문맥에 따라서 변수명도 달라질 수 있는군요. title로 수정하였습니다

@jxmen
Copy link
Author

jxmen commented Aug 13, 2021

0eee1cf git rebase -i HEAD~2로 커밋을 합치는 작업이 잘 들어갔네요. git을 맨날 add, commit, push, pull 등만 사용하다가 이런 것들이 있는지도 몰랐네요. git도 앞으로 공부할 것들이 꽤 있을 것 같아요

TaskMapper를 TaskManager에서만 호출하도록 변경합니다.
TaskManager는 TaskMapper의 결과를 그대로 리턴합니다.

추가로 현재 HTTP DELETE 메서드의 경우 204 No Content를 반환하는데,
이는 response body가 없으므로 삭제하는 메서드를 void 타입으로 변경하였습니다.
객체지향 생활체조 원칙 - 원시타입과 문자열을 포장

HttpRequest의 path를 클래스로 분리하고 관련 테스트 코드를 추가하였습니다.
객체지향 생활체조 원칙 - 원시타입과 문자열을 포장

HttpRequest의 path를 클래스로 분리하고 관련 테스트 코드를 추가하였습니다.
@jxmen
Copy link
Author

jxmen commented Aug 13, 2021

08.13

  • TaskHttpHandler에서 TaskMapper의 의존성을 제거하였습니다.
    • TaskManager에서 TaskMapper를 의존하도록 하였습니다.
  • HttpRequest에서 path를 클래스로 포장하였고 단위 테스트를 작성했습니다.
  • Task들을 담는 컬렉션을 hashMap으로 변경하고 일급 컬렉션으로 따로 분리하였습니다.

이제 쉬운 것부터 단위 테스트를 하나씩 추가해 보려 합니다..ㅎㅎ


public class TaskMap {

private final Map<Long, Task> taskMap = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

HashMap에 저장을 하고 계시군요 ㅎㅎ 만약 할 일 목록을 등록한 순서대로 혹은 사용자가 원하는 순서대로 얻고 싶다면 어떻게 해야 할까요?

Copy link
Author

Choose a reason for hiding this comment

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

HashMap에서 범위검색 혹은 정렬이 필요할 경우 TreeMap을 고려해 볼 수 있을 것 같습니다

Comment on lines 23 to 37
HttpRequest httpRequest = createHttpRequest(httpExchange);
String body = getParsedResponseBody(httpExchange);

long taskId = httpRequest.getTaskIdFromPath();
if (httpRequest.isReadAll()) {
new HttpResponseOK(httpExchange).send(taskManager.getAllTasks());
}

if (httpRequest.isReadOne()) {
try {
new HttpResponseOK(httpExchange).send(taskManager.findTaskWith(taskId));
} catch (TaskIdNotFoundException error) {
new HttpResponseNotFound(httpExchange).send(error.getMessage());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이런 모양으로 만들면 어떨까 고민해 봤어요

public void handle(HttpExchange httpExchange) throws IOException {
        HttpRequest httpRequest = createHttpRequest(httpExchange);
        httpRequest.handleRequest();
        return createResponse(httpRequest);
}

Copy link
Author

Choose a reason for hiding this comment

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

HttpResponse에서 content를 필드로 변경하고, HttpRequest에서 HttpResponse를 만들어 전달하는 메서드를 만들고 핸들러에서 이를 보내도록 바꾸어보았습니다.

HttpResponse의 content는 필드로 분리
Comment on lines +30 to +34
String replaced = path.getTaskId();

if (!replaced.isEmpty()) {
return Long.parseLong(replaced);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

replaced는 무엇을 의미하나요? id라고 하는 것은 어떨까요?

Comment on lines +62 to +67
Task task = taskMap.get(id);
if (task == null) {
throw new TaskIdNotFoundException();
}

return task;
Copy link
Contributor

Choose a reason for hiding this comment

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

return Optional.ofNullable(taskMap.get(id))
                .orElseThrow(TaskIdNotFoundException::new);

이렇게 작성하는 방법도 있긴한데, 엄청 권하지는 않습니다

Comment on lines +48 to +49
String newTitle = content.getTitle();
task.setTitle(newTitle);
Copy link
Contributor

Choose a reason for hiding this comment

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

우변의 getTitle이 이미 title이라는 의미를 가지고 있기 때문에, newTitle이라는 변수를 만들지 않아도 이해가 될 것 같아요. 좌변의 newTitle이 오히려 이해를 방해하는 느낌이에요.

task.setTitle(content.getTitle());

@hannut91
Copy link
Contributor

저 혼자서 작업 할때는 안할 수도 있고, 혹은 틀린 방향으로 갈 수도 있습니다. 하지만 코드숨에서 만큼은 틀린 부분을 바로잡을 수 있게 방향성을 제시해 주실 수 있기 때문에, 그만큼 더 다양한 시도를 해볼 수 있던 것 같습니다.

맞습니다 ㅎㅎ 학습할 때 주의해야 할 것 중 하나가 실수를 학습하는 것입니다. 코드숨 과정에서는 트레이너들이 피드 포워드를 통해 도와드리니 학습하기에 아주 좋은 환경입니다. 그리고 이런 환경을 만드는 것은 아주 어렵죠. 많은 시도를 하고 많은 것을 배워가셨으면 좋겠습니다.

고생 많으셨습니다

@hannut91 hannut91 merged commit 38bdd1d into CodeSoom:pangnem Aug 16, 2021
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.

None yet

2 participants