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 만들기 #71

Merged
merged 5 commits into from Dec 6, 2021
Merged

Todo REST API 만들기 #71

merged 5 commits into from Dec 6, 2021

Conversation

ririti
Copy link

@ririti ririti commented Dec 2, 2021

아직 자바에 익숙하지 않아서 책을 보면서 좀더 생각을 해보고 있습니다.

구현 능력이 떨어지다보니 좀더 노력해야할 거같아요

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

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

안녕하세요. 반갑습니다!

app/src/main/java/com/codesoom/assignment/App.java Outdated Show resolved Hide resolved
Comment on lines 38 to 40
String method = exchange.getRequestMethod();
URI uri = exchange.getRequestURI();
String path = uri.getPath();
Copy link
Collaborator

Choose a reason for hiding this comment

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

변경하지 않을 값을 선언하게 될 경우엔 final을 붙여 보면 어떨까요?

Suggested change
String method = exchange.getRequestMethod();
URI uri = exchange.getRequestURI();
String path = uri.getPath();
final String method = exchange.getRequestMethod();
final URI uri = exchange.getRequestURI();
final String path = uri.getPath();

import com.sun.net.httpserver.HttpExchange;
import com.sun.net.httpserver.HttpHandler;

import java.io.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

import *을 하지 않는 것이 좋은 습관일 수 있어요. 어떤 것을 import하는지 분명히 명시해 주는 연습을 해 봅시다. IDE의 기능을 사용하면 *을 각 import 문으로 확장해줄 수 있을 거에요.

Copy link
Author

Choose a reason for hiding this comment

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

IDE기능을 꺼두고 각각 어떤 기능것을 명시해주는지 알도록 앞으로는 *쓰는것을 자제하겠습니다

import * 해제하 각항목에 대해 명시(import문 확장)
@ririti
Copy link
Author

ririti commented Dec 3, 2021

변경하지 않는 값은 상수로 바꾸고
import*을 삭제하고 각 import문으로 확장했습니다

app/src/main/java/com/codesoom/assignment/App.java Outdated Show resolved Hide resolved
import java.util.stream.Collectors;

public class DemoHttpHandler implements HttpHandler {
private ObjectMapper objectMapper = new ObjectMapper();
Copy link
Collaborator

Choose a reason for hiding this comment

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

이번 과제에서는 ObjectMapper를 사용하지 않고 작업을 해 보면 어떨까요? 추후 아샬님의 강의 단계에서 등장할 거에요.

Copy link
Author

Choose a reason for hiding this comment

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

ObjectMapper를 사용하지 않는 방법은 프레임워크를 통해서가 아닌 따로 자바의 기능내에서 말씀하는건가요??

Copy link
Collaborator

Choose a reason for hiding this comment

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

ObjectMapper를 쓰지 않고 직접 String을 갖고 작업하는 방법을 권해드리는 거에요. 물론 계속 쓰는 것이 더 낫다고 생각하시면 그대로 사용하셔도 괜찮아요.

Copy link
Author

Choose a reason for hiding this comment

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

넵 감사합니다. 다음 과제부터와 이번과제에서부터 적용해보도록 노력하겠습니다.

import * 해제하 각항목에 대해 명시(import문 확장)
# Conflicts:
#	app/src/main/java/com/codesoom/assignment/DemoHttpHandler.java
import * 해제하 각항목에 대해 명시(import문 확장)
@ririti
Copy link
Author

ririti commented Dec 4, 2021

오늘 올라온 강의를 보고 과제를 마무리하면서 자바가 너무나도 약하다는걸 다시한번 깨달았습니다.
과제를 마무리하고 상수선언과 변하지 않는 값을 final 바꾸었습니다.

Comment on lines +77 to +85
if (method.equals("GET") ) {
handleDetail(exchange, task);
}

if (method.equals("PUT") || method.equals("PATH")) {//PUT : 전체 PATH : 일부분
handleUpdate(exchange, task);
}

if (method.equals("DELETE") ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

GET, PUT, DELETE 같은 HTTP method는 enum으로 선언해보면 어떨까요? 문자열을 그대로 사용하는 것은 아쉬운 일입니다.

}

private void handleList(HttpExchange exchange) throws IOException {
send(exchange, 200, toJSON(tasks));
Copy link
Collaborator

Choose a reason for hiding this comment

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

200 같은 HTTP status 코드도 enum으로 선언해 보세요.

Comment on lines +142 to +149
//get을 사용시 그대로 나오며 값이 없다면 excption을 준다
private Task findTask(Long id) {
return tasks.stream()
.filter(task -> task.getId().equals(id))
.findFirst()
.orElse(null);
// .findFirst().get();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

주석에서는 값이 없다면 exception을 준다라고 작성하셨는데요, 코드에서는 값이 없는 경우에 null을 리턴하도록 하고 있습니다. 주석이나 코드 둘 중 하나를 고칠 필요가 있겠어요.

Comment on lines +167 to +170
private Long generateId() {
newId += 1;
return newId;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

synchronized에 대해 조사해 보시고 적용을 고려해 보세요.

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

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

한 주간 고생 많으셨습니다. 다음주부터는 일요일 저녁에 주간 회고를 작성해 공유해 주세요.

@ririti
Copy link
Author

ririti commented Dec 5, 2021

이번주 과제는 끝났지만 받은 리뷰는 틈틈히 고쳐 보도록 하겠습니다!
그리고 이번주 주간회고도 작성해서 공유하였습니다

@johngrib johngrib merged commit 98b29d1 into CodeSoom:ririti Dec 6, 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