2주차과제 - SpringWeb ToDo REST API #115

tmxhsk99 · 2023-07-10T10:33:53Z

안녕하세요
2주차 과제 - Spring Web으로 ToDo REST API 만들기
리뷰 요청드립니다.
그런데 git CI e2e 테스트가 로컬에서는 성공하였는데 git CI 에서는 실패가나네요...
제가 뭘 잘못한건지...
추가적으로 질문이 있습니다 혹시 주석은 어떻게 작성하는 게 좋나요?
원래 저는 클래스랑 메서드에 다 붙여줬었는데
이전에 스터디할때 주석은 소스랑 안맞으면 오히려 혼란을 줄 수 있어서 잘 안쓰고 쓴다면 실제 누구의 지시로 언제 개발 이렇게만 남기신다고 하더라구요
그런데 여기 다른분들 피드백보면 주석관련해서도 피드백이 있더라구요...
혹시 어느정도 주석을 어떻게 남겨야할 지 궁금합니다
감사합니다.

- 할일 추가 기능추가 - 할일 조회 리스트 기능추가 - 할일 상세조회 기능 추가 - 할일 수정 기능 추가 - 할일 삭제 기능추가

hannut91 · 2023-07-11T01:37:51Z

git CI e2e 테스트가 로컬에서는 성공하였는데 git CI 에서는 실패가나네요...

이건 Git CI 코드에 문제가 있는 것 같네요. 수정해야겠네요. 로컬에서 성공하면 일단 신경쓰지 않으셔도 돼요 ㅎㅎ

hannut91 · 2023-07-11T01:44:53Z

주석은 어떻게 작성하는 게 좋나요? 원래 저는 클래스랑 메서드에 다 붙여줬었는데 이전에 스터디할때 주석은 소스랑 안맞으면 오히려 혼란을 줄 수 있어서 잘 안쓰고 쓴다면 실제 누구의 지시로 언제 개발 이렇게만 남기신다고 하더라구요

주석이 소스랑 안맞으면 오히려 혼란을 줄 수 있다는 것은 저도 동의를 합니다. 소스 코드를 작성하고나서 주석을 작성했는데, 혹은 반대로 주석을 작성하고 나서 소스 코드를 작성했는데 요구 사항의 변경이 있어서 소스 코드를 수정합니다. 근데 주석을 안고치면 주석과 코드가 달라지게 되고 혼란을 줄 수 있죠. 그래서 주석도 중복이기 때문에 제거하고 주석이 없어도 소스 코드를 이해할 수 있도록 만드는게 중요하다고 많이 얘기합니다.

하지만 그렇다고 해서 주석을 달면 안된다라고 이해하시면 안됩니다. 주석은 소스 코드로 표현할 수 없는 부분을 표현 가능하게 해주기 때문에 잘 사용해야 합니다.

예를들어서 결제를 하고나서 사용자의 결제 수단을 저장하는 기능이 있다고 고려해 봅시다. 그런데 결제 수단을 저장하는 것이 어떠한 이유로 실패할 수 있겠죠. 결제 수단 저장이 실패해도 결제는 가능해야 하기 때문에 다음과 같이 할 수 있습니다.

try {
  savePaymentMethod();
} catch (e: Exception) {
  // 결제 수단 저장에 실패해도 결제는 가능해야 하기 때문에 에러를 무시합니다.
}

그리고 문서화는 소프트웨어 개발에서 중요한 부분이기 때문에 문서화를 잘하는 것도 중요합니다. 그전에 먼저 좋은 문서를 읽어보는 것을 권해드립니다. 우리가 흔히 쓰는 ArrayList부터 읽어보세요. cmd + 클릭하면 소스 코드를 볼 수 있고 잘 문서화된 예제를 보실 수 있습니다.

hannut91 · 2023-07-11T01:46:06Z

app/src/main/java/com/codesoom/assignment/application/TaskService.java

+    }
+
+    public void deleteTask(Long id) {
+        Task task = taskRepository.findById(id).orElseThrow(TaskNotFound::new);


위에 만들어진 getTask를 사용할 수도 있겠네요

hannut91 · 2023-07-11T01:47:17Z

app/src/main/java/com/codesoom/assignment/repository/infra/TaskListRepositoryImpl.java

+    }
+
+    private Long generateId() {
+        return ++taskId;


전위연산자를 사용해서 현재의 값을 반환하고 나서 값을 증가시키고 있는 것 같아요. 사용법을 정확히 알고 있다면 ++value 와 value++ 을 구분해서 사용할 수 있겠지만, 사실 조금 헷갈릴수도 있는 문법이에요. 우리가 한 번 더 생각해야되는거죠. 아주 조금이라도 헷갈릴 수 있거나 다르게 해석될 여지가 있다면 저는 사용을 지양하는 것이 좋다고 생각해요.

그러면 taskId = taskId +1 나 += 를 사용하는게 좋을것 같으시다는 말씀인가요?

네 맞습니다

hannut91 · 2023-07-11T01:48:34Z

app/src/main/java/com/codesoom/assignment/repository/infra/TaskListRepositoryImpl.java

+    }
+
+    private Long generateId() {
+        return ++taskId;


만약에 한 번에 요청이 10개가 왔다고 가정해 봅시다. 기대했던대로 아이디가 만들어질까요? 어떻게 될지 가설을 세워보고 어떻게 검증할지 계획을 세워봅시다

기대했던 대로는 아이디가 안 만들어질것 같습니다.

찾아보니 증감연산자는 하나의 연산처럼 보이지만 여러개의 연산으로 이루어져 있다고합니다.

메모리에서 변수 i의 현재 값을 읽습니다.

그 값을 1 증가시킵니다.

증가된 값을 다시 변수 i에 저장합니다.
단일 연산이 아니므로 멀티스레드 환경에서 원자성을 보장하지 못합니다. (연산 사이에 다른 스레드가 접근하므로...)
간단히 생각하면 해당 블록에 synchronized 걸면 되지만 대기하면서 노는 스레드가 많아져서 느려집니다.

멀티쓰레드 어플리케이션에서의 volatile 를 선언하지 않은 이외 변수에 대한 작업은 성능상의 이유로 CPU 캐시를 이용한다고 합니다 캐시에 읽고 쓰기한 후에 메인 메모리에 반영이 쓰기작업이 바로 보장되지 않아 스레드별로 변수가 다르게 나타날수 있습니다.

이것이 가시성인데 가시성(여러스레드에 수정결과가 안보임)은 volatile 을 붙여주면 해결을 되지만 (캐시를 안바라보고 메인메모리에서만작업)
그러나 이 방법도 동시에 메인메모리에 접근하면 의미가 없습니다.

그러므로 자바에서 제공하는 java.util.concurrent.atomic 패키지 하위의 기능을 써야할 것 같습니다.
java.util.concurrent.atomic 의 경우 현재 스레드가 존재하는 CPU의 CacheMemory와 MainMemory에 저장된 값을 비교하여, 일치하는 경우 새로운 값으로 교체하고, 일치하지 않을 경우 기존 교체가 실패되고, 이에 대해 계속 재시도하는 방식입니다.

그러므로 AtomicLong을 써서 해결해야할 것 같습니다.

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/package-summary.html

왜 기대했던 대로 안되는지 원인도 파악하셨고 여러가지 대안도 찾아보았고 그중에서도 어떤 방법을 선택했는지 그 이유에 대해서도 정리를 해주셨네요.

실제로 실험해보는 것도 의미가 있을 것 같아요. 어떻게 검증할 것인가도 중요한 부분이라서요. API 테스트를 조금 수정해서 한 번에 여러개 요청이 가도록해서 테스트 해봤습니다.

describe('POST /tasks', () => { it.only('responses 201 Created', async () => { const promise = []; for (let i = 1; i < 100; i++) { promise.push( frisby.post('/tasks', { title }) .expect('status', 201), ); } await Promise.all(promise); }); });

synchronized를 걸은 경우

hannut91 · 2023-07-11T01:51:13Z

app/src/main/java/com/codesoom/assignment/controllers/TaskController.java

+        if(id == null) {
+            return ResponseEntity.notFound().build();
+        }


어떤 요청에서 id가 null이 되나요?

그냥 http DELETE http://localhost:8080/tasks/ 이런 식으로 요청하면 null로 들어오겠지라고
생각했는데 HttpRequestMethodNotSupportedException 에러로 떨어지네요
수정하겠습니다

hannut91 · 2023-07-11T01:52:21Z

app/src/main/java/com/codesoom/assignment/controllers/ExceptionController.java

+    }
+
+    private ResponseEntity<ErrorResponse> parseErrorResponseEntity(CustomBaseException e, ErrorResponse errorResponse) {
+        return ResponseEntity.status(e.getStatusCode()).body(errorResponse);


에러 객체에서 상태 코드를 결정하는 군요. 저는 예외는 예외만 나타내고 응답을 어떻게 줄지 Advice에서 결정하도록 했었는데 어떤게 더 좋을가요?

커스텀 예외 객체의 status 값을 실제 응답 status 로 쓰는 형태인데
커스터 예외 객체는 예외에 관한 정보만 두고
Advice에서 해당 예외를 체크해서 상태코드를 결정하시라는 말씀이신가여?
분리하는게 더 좋아보이기는 합니다 예외는 예외 응답은 응답은 Advice에서 처리하는게 맞다고 생각은 되는데
그렇게 하면 뭔가 또 if문으로 분기처리하는 게 길어질것 같아서
고민되긴 하네요. (뭔가 분기가 많은 if를 꺼리게되는 느낌)
혹시 좋은 방법이 있을까요?

Advice에서 핸들러에 @ResponseStatus(HttpStatus.�NOT_FOUND)를 붙여서 어떻게 응답할지 결정할 수 있어요

See also

https://spring.io/blog/2013/11/01/exception-handling-in-spring-mvc/

이런 좋은 기능이 있었네요!
다음부터 일단 공식사이트 먼저 검색해봐야겠습니다.

hannut91 · 2023-07-12T01:46:13Z

app/src/main/java/com/codesoom/assignment/repository/infra/TaskListRepositoryImpl.java

+    @Override
+    public Optional<Task> findById(Long id) {
+        return taskList.stream()
+                .filter(t -> t.getId().equals(id))
+                .findFirst();
+    }


update에서 하고 있는 예외를 던지는 것을 여기서 하는 것은 어떨까요?

수정

hannut91 · 2023-07-13T01:27:49Z

app/src/main/java/com/codesoom/assignment/controllers/TaskController.java

+    public ResponseEntity<Task> create(@RequestBody TaskCreate taskCreate) {
+        taskCreate.validate();
+        Task task = taskService.createTask(taskCreate);
+        return ResponseEntity.status(HttpStatus.CREATED).body(task);


ResponseEntity객체를 이용해서 응답 상태 코드를 변경하셨네요. @ResponseStatus를 사용해서 상태 코드를 지정할 수 있습니다.

아마 이런 어노테이션이 있는지 몰라서 그러셨을 것 같아요. 아주 지극히 정상입니다.

어노테이션이란 코드 외적으로 코드의 힌트를 추가하는 메타 프로그래밍의 일종으로 볼 수 있습니다. 코드에는 영향을 줄 수는 없지만 코드에 대한 메타 정보를 입력할 수 있습니다. 그래서 어노테이션의 장점은 기존 코드에 아무런 영향을 주지 않는다는 것입니다.

그런데 어노테이션 프로세서에게는 의미가 있습니다. 어노테이션 처리자는 어노테이션의 값을 읽어서 다르게 처리합니다. 즉 코드를 읽을 때 우리가 읽는 레이어를 여러개가 있다고 바라볼 수 있어야 합니다.

잘쓰면 편리하지만 아주 치명적인 단점이 있습니다. 발견 가능성이 아주 낮다는 거예요. 무슨 말이냐면 이러한 어노테이션이 있다는 것을 외우지 않는 이상 알 수가 없습니다. 그래서 어노테이션을 지양하는 사람도 있어요.

See also

https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/bind/annotation/ResponseStatus.html

맞는것 같습니다 누가 알려주거나 쓰는것을 보는게 아닌 이상
뭐가 있는지 알기 어렵더라구요...
이런게 있는게 알려면 공식문서를 보는수 밖에 없겠져...?

네 ㅎㅎ 공부만이 답입니다~!

hannut91 · 2023-07-13T01:35:25Z

app/src/main/java/com/codesoom/assignment/controllers/TaskController.java

+    @DeleteMapping("/tasks/{id}")
+    public ResponseEntity<Void> delete(@PathVariable Long id) {
+        taskService.deleteTask(id);
+        return ResponseEntity.noContent().build();
+    }


왜 NO_CONTENT로 응답을 해야 하는지 다음 문서를 읽어보세요.

RFC 2616 - 9.7 Delete

삭제된 경우 클라이언트에게 전달할 데이터가 없으므로 203 (NO_CONTENT) 로 보내는것 같습니다.
삭제하더라도 클라이언트가 삭제한 데이터가 필요할 수 있는 경우는 200(OK)를 내려주면서 삭제한 값도 본문에 첨부하는것 같습니다. ( 내부 정책마다 유도리있게 쓸려고 )
그런 차이 때문에
특별한 경우가 아니면 네트워크 효율성을 위해 기본적으로 203을 내보내는 것 같습니다.

204를 말씀하신거겠죠? 맞습니다 ㅎㅎ

앗 204 입니다 !

hannut91 · 2023-07-14T02:32:19Z

app/src/main/java/com/codesoom/assignment/controllers/TaskController.java

+        return taskService.updateTask(task);
+    }
+
+    @ResponseStatus(value = HttpStatus.NO_CONTENT)


@ResponseStatus(HttpStatus.NO_CONTENT)

아마 위의 코드와 일관성을 맞출려고 하신 것 같네요. 요렇게 쓰셔도 됩니다 ㅎㅎ

hannut91 · 2023-07-14T02:35:45Z

app/src/main/java/com/codesoom/assignment/repository/infra/TaskListRepositoryImpl.java

+
+@Repository
+public class TaskListRepositoryImpl implements TaskRepository {
+    private ArrayList<Task> taskList = new ArrayList<>();


무엇을 공부해야 하나 고민될 때 좋은 접근 방법 중 하나는 내가 자주 쓰는 것부터 공부하는 것입니다. ArrayList의 Javadoc문서를 읽어보고 어떻게 동작하는지 한 번 알아보세요.

그리고 데이터가 아주커졌을 때 수정하는 일이나 삭제하는 일이 너무 느려지지 않을까 고민도 해보시면 좋을 것 같아요

깊게 이해하려면 어떻게 하나 고민이었는데
좋은방법인것 같습니다 한번 해당 방법 적용해서 공부해보겠습니다!

hannut91

이번 한 주도 고생 많으셨습니다!

feat : spring web todo api 작성

3ef4bea

- 할일 추가 기능추가 - 할일 조회 리스트 기능추가 - 할일 상세조회 기능 추가 - 할일 수정 기능 추가 - 할일 삭제 기능추가

tmxhsk99 changed the title ~~2주차과자 - SpringWeb ToDo REST API~~ 2주차과제 - SpringWeb ToDo REST API Jul 10, 2023

tmxhsk99 added 4 commits July 10, 2023 19:36

refactor : 사용하지 않는 검증 메서드 삭제

d5336c5

refactor : CustomExcpetion 처리 로직 책임 분리

27b1719

build: gradle 버전 수정

7fcdc19

build: 패키지관련 및 build.gradle 의존성추가

ebbbbec

tmxhsk99 force-pushed the todo-springweb-apply branch from 0b9d7df to ebbbbec Compare July 10, 2023 22:00

hannut91 reviewed Jul 11, 2023

View reviewed changes

tmxhsk99 added 3 commits July 12, 2023 03:07

refactor : getTask를 호출로 로직 변경

5b942e7

fix : taskId Long 타입에서AtomicLong으로 변경

ec8d6e0

refactor : 사용되지 않는 검증 로직 삭제

af5b834

hannut91 reviewed Jul 12, 2023

View reviewed changes

refactor : 에러 핸들러status 처리 로직 분리, findById 내부에서 에러치리하도록

d488924

수정

hannut91 reviewed Jul 13, 2023

View reviewed changes

refactor : ResponseStatus 어노테이션 사용으로 변경

c7b8cc5

hannut91 reviewed Jul 14, 2023

View reviewed changes

refactor : ResponseStatus value 값 생략

12d0811

hannut91 approved these changes Jul 16, 2023

View reviewed changes

hannut91 merged commit b95c02b into CodeSoom:tmxhsk99 Jul 18, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2주차과제 - SpringWeb ToDo REST API #115

2주차과제 - SpringWeb ToDo REST API #115

tmxhsk99 commented Jul 10, 2023 •

edited

Loading

hannut91 commented Jul 11, 2023

hannut91 commented Jul 11, 2023

hannut91 Jul 11, 2023

hannut91 Jul 11, 2023

tmxhsk99 Jul 11, 2023

hannut91 Jul 12, 2023

hannut91 Jul 11, 2023

tmxhsk99 Jul 11, 2023

hannut91 Jul 12, 2023

hannut91 Jul 11, 2023

tmxhsk99 Jul 11, 2023

hannut91 Jul 11, 2023

tmxhsk99 Jul 11, 2023

hannut91 Jul 12, 2023

tmxhsk99 Jul 12, 2023

hannut91 Jul 12, 2023

hannut91 Jul 13, 2023

tmxhsk99 Jul 13, 2023 •

edited

Loading

hannut91 Jul 14, 2023

hannut91 Jul 13, 2023

tmxhsk99 Jul 13, 2023

hannut91 Jul 14, 2023

tmxhsk99 Jul 14, 2023

hannut91 Jul 14, 2023

hannut91 Jul 14, 2023

tmxhsk99 Jul 14, 2023

hannut91 left a comment

2주차과제 - SpringWeb ToDo REST API #115

2주차과제 - SpringWeb ToDo REST API #115

Conversation

tmxhsk99 commented Jul 10, 2023 • edited Loading

hannut91 commented Jul 11, 2023

hannut91 commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmxhsk99 Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannut91 left a comment

Choose a reason for hiding this comment

tmxhsk99 commented Jul 10, 2023 •

edited

Loading

tmxhsk99 Jul 13, 2023 •

edited

Loading