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

1주차 과제 - ToDo REST API 만들기 #115

Merged
merged 34 commits into from Aug 7, 2022
Merged

Conversation

jdalma
Copy link

@jdalma jdalma commented Aug 1, 2022

안녕하세요
첫 과제 진행이라 긴장되네요 ㅎㅎ
잘 부탁드립니다

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.

만나서 반갑습니다. 화이팅!

jdalma and others added 2 commits August 2, 2022 08:47
Co-authored-by: JohnGrib <johngrib82@gmail.com>
public class TaskHandler implements HttpHandler {

public static List<Task> tasks = new LinkedList<>();
public ObjectMapper mapper = 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

@jdalma jdalma Aug 3, 2022

Choose a reason for hiding this comment

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

LazyHolder를 사용해서 싱글톤 TaskConverter를 만들어보았습니다

Copy link
Collaborator

@johngrib johngrib Aug 3, 2022

Choose a reason for hiding this comment

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

앗 추가 과제를 잘못 이해하신 것 같아요. 제가 여기에서라고 한 것은 이 프로젝트에서를 말합니다. ObjectMapperTaskConverter로 이동했을 뿐 여전히 사용하고 있습니다.

Copy link
Author

Choose a reason for hiding this comment

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

아 JSON ↔︎ Task를 직접 해보라고 하셨던거군요 ㅎㅎ
Reflection을 이용해서 Task필드에 직접 접근은 가능할 것 같은데 JSON을 잘 구분해내는게 문제 일 것 같네요
일단 한번 해보겠습니다!

Copy link
Author

Choose a reason for hiding this comment

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

JSON문자열로 Task객체 만들기를 시도 중 입니다
정규표현식을 찾아보면서 생각나는대로 해봤습니다 ㅎㅎ

7747125

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 과제는 코드숨 메인 과제가 아니므로 꼭 달성하지 않으셔도 괜찮습니다. 하지만 해보시면 좋은 경험을 얻으실 수 있을 거라 생각해요. 한편 팁을 드리자면 조악하게라도 먼저 돌아가게 완성한 다음, 그 다음에 버전업을 한다고 생각하고 개선하는 방향으로 작업해보시는 것을 권해드리고 싶습니다. 그래서 처음엔 리플렉션을 안 쓰셔도 괜찮아요. 일단 Task를 위해 하드코딩해보는 거죠. 그리고 그게 돌아가면 좀 더 넓은 케이스를 커버할 수 있도록 고치고, 그게 돌아가는 걸 확인하면 더 넓은 케이스를 커버할 수 있게 고치는 걸 반복하는 겁니다. 그리고 이걸 많이 하다보면 자신만의 라이브러리가 되는 거죠.

Comment on lines 11 to 13
public void setId(Long id) {
this.id = id;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 클래스에서 setter를 제거하고 과제를 진행하는 것도 가능할지 생각해 봅시다.

Copy link
Author

Choose a reason for hiding this comment

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

setter를 제거하고 Task 생성자로만 통해서 값을 넣어보았습니다

Task task = taskConverter.convert(body);
Task newTask = new Task(id, task.getTitle());

Task를 2개 만들어서 처리했는데.. 이런 경우는 어떻게 처리하는게 좋을까요??

Copy link
Collaborator

Choose a reason for hiding this comment

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

newTask를 리턴해보세요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다. 이번 기회에 함께 공부할만한 건이 있습니다. "팩토리"에 대해 조사해 보세요. 이걸 공부할 땐 코드 예제보다 어떤 필요에서 비롯되었고, 어떤 개념을 표현하는 것인지에 집중해서 정리해 보도록 해보세요.

Copy link
Author

@jdalma jdalma Aug 7, 2022

Choose a reason for hiding this comment

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

팩토리에 대해 조사하다가 종립님의 글도 보게 되었네요 ㅎㅎ
도메인 주도 설계와 연관이 있는 것 같아 뭔가 거대한 문을 열게되는 느낌이네요
더 조사하고 정리하여 꼭 공유 드리도록 하겠습니다!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

꼼꼼하게 공부하고 다양하게 실험해 보시길 바랍니다. 거대한 문을 열게 되는 느낌이라니 저까지 두근두근하는군요. 화이팅!

enum에 문자열을 받아 비교하는 equals 메서드 추가
ObjectMapper 제거 후 JSON → Task를 진행해 보았습니다
Comment on lines 58 to 68
public Map<String , String> jsonToMap(String content){
Map<String , String> map = new HashMap<>();
String[] contents = content.split(",");
Pattern pattern = Pattern.compile("\"(.*?)\"");
for(String test : contents){
Matcher matcher = pattern.matcher(test);
String key = matcher.find() ? matcher.group().replace("\"" , "") : "";
String value = matcher.find() ? matcher.group().replace("\"" , "") : "";
map.put(key , value);
}
return map;
Copy link
Collaborator

Choose a reason for hiding this comment

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

너무 좋습니다. 시도도 좋습니다. 다만 이 메소드의 동작을 어딘가에는 명시해두는 것이 좋습니다. "이런 입력이 들어가면 이런 것이 나온다"는 걸 알려주는 거죠. 시간이 부족하다면 주석이나 README 문서도 괜찮습니다. 하지만 시간이 좀 있다면 테스트코드로 이 메소드의 사용 방법을 기록으로 남기는 것을 꼭 해보시길 바랍니다.

// objectMapper.writeValue(outputStream , new ArrayList<>(tasks.values()));
return "";
public String convert(Task task){
return String.format("{\"id\":%d,\"title\":\"%s\"}" , task.getId() , task.getTitle());
Copy link
Author

Choose a reason for hiding this comment

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

Task → JSON으로 변경하는 메서드를 Task모델에 넣을지 TaskConverter에 넣을지 고민해 보았습니다
Task에 대한 Converter가 이미 존재하니 TaskConverter에 작성해 보았습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다. 빠른 구현을 목표로 한 작업이라는 것이 눈에 보입니다. 이 정도로도 과제의 목표는 잘 달성하신 것 같습니다. 만약 여기에서 한 단계 더 어려운 것을 해보고 싶다면 (직접 코딩을 하지 않고) task에 프로퍼티가 하나 더 추가되었을 때 / 100개가 더 추가되었을 때 어떻게 대응할지에 대해 상상해 보는 연습을 해보세요.

String key = matcher.find() ? matcher.group().replace("\"" , "") : "";
String value = matcher.find() ? matcher.group().replace("\"" , "") : "";
String key = getContent(matcher);
String value = getContent(matcher);
Copy link
Author

Choose a reason for hiding this comment

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

삼항 연산자로 중복되는 코드를 getContent()로 분리해 보았습니다

// when
Map<String , String> map = converter.jsonToMap(json);

// then
Copy link
Collaborator

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.

테스트 코드를 처음 작성해봐서 어떻게 해야할까... 고민하다가 종립님이 말씀하신 "이런 입력이 들어가면 이런 것이 나온다"에 초점을 맞추어서 작성해 보았습니다 ㅎㅎ

ddddde8

Copy link
Collaborator

Choose a reason for hiding this comment

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

오 너무 잘 하셨어요. 이건 몇 주 후에 구체적으로 알려드리려 했는데 스스로 먼저 시도하셨네요. 이런 방식의 테스트 코드를 작성할 때 핵심 아이디어는 바로 우리가 어떤 함수/메소드를 만나건 간에 "뭐가 들어가서 뭐가 리턴되는지만 구체적으로 알 수 있다면" 코드를 아주 빠르게 이해할 수 있다는 것입니다. 이건 어떤 언어로 작업하건 간에 거의 공통이죠. 심지어 컴퓨터나 프로그래밍이 아닌 경우에도 통합니다. 은행 창구에 가서 새로운 계좌를 만들겠다고 하면 어떤 결과가 리턴될지 우리가 잘 알고 있는 것과 비슷합니다.

그래서 이런 쪽에 집중하는 방식에서는 입력과 리턴값에 대한 설명을 특히 중요하게 여깁니다.

좀 이른 감이 있긴 하지만 이 문서도 한번 읽어보세요. 제가 작성한 문서입니다.

https://johngrib.github.io/wiki/junit5-nested/

if(pathVariable == null){
throw new ParameterNotFoundException("not existing pathVariable");
}
public String getPathVariable() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기에서 생각해볼만한 주제가 하나 있습니다.

pathVariableprivate final로 선언되어 있습니다. 이걸 public으로 선언하면 getPathVariable을 굳이 만들지 않아도 됩니다. final이므로 값이 변경되거나 오염될 염려도 없죠.

Copy link
Author

Choose a reason for hiding this comment

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

언제부터인지 클래스 필드에 직접 접근은 절대 안돼라고 생각한 것 같습니다
말씀대로 변경되거나 오염될 염려가 없다면 getter를 제거하고 직접 접근을 사용하겠습니다 ㅎㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

팀 단위로 일할 때에는 팀에서 정한 컨벤션이 더 중요하다는 것도 잊지 마세요!

Comment on lines 36 to 48
HttpResponse httpResponse = HttpResponse.NO_CONTENT;
if(path.hasResource() && path.resourceEquals("tasks")){
if(path.hasPathVariable()){
handleUsingTaskId(exchange , method , Long.parseLong(path.getPathVariable()));
}
else{
handleTask(exchange , method);
}
return;
}

send(exchange , httpResponse.getCode() , "");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기에서 if 안쪽에 if 를 두지 않도록 고쳐봅시다. early return 에 대해 조사해 보시고, 여기에 적용하는 연습을 해 보세요.

Copy link
Author

Choose a reason for hiding this comment

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

early return을 읽어보고 수정해 보았습니다
훨씬 깔끔해진 것 같습니다 ㅎㅎ

b4f1a46

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다. 한편 return이 필요없어 보여도 추가해준다면 해당 시점부터 메소드가 끝난다는 것을 명시적으로 표현할 수 있어서 코드를 읽는 사람들이 더 쉽게 로직을 이해할 수 있게 될 거에요.

}

@Test
@DisplayName("JSON문자열에 키가 중복되지 않는다면 잘 가져온다")
Copy link
Collaborator

Choose a reason for hiding this comment

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

잘 가져온다는 애매한 표현입니다. 테스트 코드를 작성할 때에는 이런 표현은 사용하지 않는 것이 좋습니다. 분명하게 표현하도록 연습해 보세요.

}

@Test
@DisplayName(" [,]로 각 객체가 구분되어 있지 않다면 첫 번째 키와 값만 가져온다")
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 말하고 있는 "각 객체"는 key value entry를 말하는 것일 겁니다.

@@ -10,8 +9,52 @@
public class TaskConverterJsonToMapTest {

@Test
@DisplayName("JSON문자열이 들어왔을 때")
void json(){
@DisplayName(" JSON형식에 맞지 않아도 [\"]로 감싸준다면 가져온다")
Copy link
Collaborator

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.

테스트 코드 @DisplayName들을 다시 작성해 보았습니다

548b1ce

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다. 한결 낫군요.

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.

이렇게 하면 다른 github 사용자를 링크하게 되어 해당 사용자에게 알림이 가게 됩니다.

image

앞으로 @를 쓸 일이 있다면 다음과 같이 백틱(`)으로 씌워주세요.

`@DisplayName`

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.

한 주간 고생 많으셨습니다. 추가 과제까지 쉽지 않으셨을텐데 열심히 도전하시는 모습이 보기 좋았습니다. 다음주도 힘내세요!

Comment on lines +56 to +60
if ("GET".equals(method.name())) {
content = taskConverter.convert(task);
} else if ("PUT".equals(method.name()) || "PATCH".equals(method.name())) {
content = updateTask(exchange, id);
} else if ("DELETE".equals(method.name())) {
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도 enum을 활용할 수 있겠죠.

@johngrib johngrib merged commit 1b88ca0 into CodeSoom:jdalma Aug 7, 2022
@jdalma
Copy link
Author

jdalma commented Aug 7, 2022

한 주간 수고하셨습니다 ㅎㅎ 감사합니다

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