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,2,3 단계 - 방탈출 예약 관리] 이든(최승준) 미션 제출합니다. #10

Merged
merged 19 commits into from
Apr 18, 2024

Conversation

PgmJun
Copy link

@PgmJun PgmJun commented Apr 16, 2024

안녕하세요 러너덕! 6기 백엔드 크루 이든입니다.
스프링부트로 프로젝트를 수행해본 경험이 조금 있어 1,2,3 단계는 무난하게 진행한 것 같습니다!
리뷰 잘 부탁드립니다!

질문 💭

Endpoint에 복수형 단어를 사용하는 이유가 궁금합니다.

크루원들과 이야기를 하던 도중 Endpoint 얘기가 나와서 자세히 알고싶게 된 부분입니다.
이 질문에 대해 한 크루원이 Rest API에 대해 자세히 학습하면 Endpoint를 왜 복수형으로 사용하는지 알게 될거다라고 해주었는데,
아직 잘 모르겠어서 질문 남겨봅니다!


컨트롤러 메서드 네이밍에 정형화된 방법이 존재하나요?

스크린샷 2024-04-16 21 14 46

고민의 코드 중 하나를 가져와보았습니다.
위 메서드는 CRUD 작업 수행이 아니라, 특정 뷰를 return하는 역할을 하는 메서드라서 명사로 네이밍을 해보았습니다.
메서드명은 동사로 작성해야한다고 하지만 꽤 자연스럽고 괜찮아 보여서 이대로 사용해도 괜찮을지 질문 남깁니다.
이 네이밍이 아니라면 showAdminPage 정도로 작성했어야 적절했을까요?

이 뿐만 아니라 대부분의 컨트롤러 메서드 네이밍 과정에서 고민이 많이 되어 어려웠습니다.
혹시 컨트롤러 네이밍에도 구글 자바 코드 컨벤션과 같은 정형화된 방법이 있는 지 궁금합니다!


서비스 레이어의 메서드와 컨트롤러의 메서드 이름이 동일해도 괜찮을까요?

추가적으로 4~9단계 미션을 수행하는 과정에서 Service 레이어가 분리된다면
컨트롤러 메서드와 서비스 메서드의 네이밍이 비슷하거나 똑같은 경우도 존재할 것 같은데 그래도 괜찮은지 궁금합니다.
러너덕은 보통 개발할 때 어떻게 네이밍을 하시나요?

ex) userId로 유저를 조회하는 API를 개발하는 경우
Controller메서드명 : findUserById
Service메서드명 : findUserById

API 명세서 📃

Method Endpoint Description File Path Controller Type
GET /admin 어드민 페이지 요청 templates/admin/index.html @Controller
GET /admin/reservation 예약 관리 페이지 요청 templates/admin/reservation-legacy.html @Controller
GET /reservations 예약 정보 @RestController
POST /reservations 예약 추가 @RestController
DELETE /reservations/{id} 예약 취소 @RestController

@PgmJun PgmJun changed the title [1,2,3 단계 - 방탈출 예약 관리] 이든(최승준) 미션 제출합니다. [1,2,3 단계 - 방탈출 어드민] 이든(최승준) 미션 제출합니다. Apr 16, 2024
@PgmJun PgmJun changed the title [1,2,3 단계 - 방탈출 어드민] 이든(최승준) 미션 제출합니다. [1,2,3 단계 - 방탈출 예약 관리] 이든(최승준) 미션 제출합니다. Apr 16, 2024
Copy link

@Deocksoo Deocksoo left a comment

Choose a reason for hiding this comment

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

안녕하세요 이든, 러너덕입니다.
필요한 것들만 간단하게 잘 구현해주셨네요 👍
몇 군데 코멘트 남겨두었으니, 확인 한 번 부탁드려요.

Endpoint에 복수형 단어를 사용하는 이유가 궁금합니다.

REST API가 무엇인가요? 웹 개발을 할 때에는 왜 REST API를 사용하나요?
이 질문들에 한번 답하다 보면 자연스럽게 이유를 찾게 될 수도 있을것같은데요 :)

컨트롤러 메서드 네이밍에 정형화된 방법이 존재하나요?

일반적으로 정형화되어있는 방법은 없어요. 컨트롤러 메서드도 어노테이션이 붙어있지만 결국 동일한 메서드에요. 특별히 다른 네이밍 방식이 있는 것이 아에요.
이전 레벨에서 학습하였던 것처럼 메서드가 하는 일을 잘 표현해줄 수 있으면서 java convention을 따르도록 하는 이름을 고민해보시면 좋겠네요 :)

서비스 레이어의 메서드와 컨트롤러의 메서드 이름이 동일해도 괜찮을까요?

메서드의 역할과 코드 일관성을 고민하면서 이름을 짓죠.
이름이 동일하면 안될 이유가 있나요~?


@GetMapping("")
public ResponseEntity<List<Reservation>> getReservationDatum() {
return ResponseEntity.ok(reservations);

Choose a reason for hiding this comment

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

응답을 전달할 때 ResponseEntity으로 감싸서 되돌려주고 계시네요. 이렇게 하신 이유가 무엇인가요?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

ResponseEntity 를 사용함으로써 Http Header, Body, Status Code를 편리하게 관리할 수 있어 사용하였습니다!

만약 ResponseEntity 를 사용하지 않고 Http Response 정보를 다루려면

@DeleteMapping("/{id}")
    public Optional<?> deleteReservationsData(@PathVariable long id, final HttpServletResponse response) {
        boolean isRemoved = reservations.removeIf(reservation -> reservation.getId().equals(id));
        if (isRemoved) {
            response.setStatus(200);
        }
        response.setStatus(404);
        return null;
    }

위 코드와 같이 API 메서드의 매개변수로 HttpServletResponse를 받아 내부적으로 respone에 대한 설정을 수행해주어야 하는것으로 알고 있는데,
이 코드보다 가독성이 높고 편리하다고 생각이 들어서, ResponseEntity를 사용하였습니다!

import org.springframework.web.bind.annotation.RequestMapping;

@Controller
@RequestMapping("/admin")

Choose a reason for hiding this comment

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

왜 RequestMapping으로 최상위 path를 추출했나요?
이렇게 하면 어떤 유용한 점이 있나요?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

endpoint에서 공통적으로 사용되는 부분을 클래스에 @RequestMapping 애노테이션으로 작성해주면
하위에 작성되는 메서드들은 Endpoint 매핑에서 "/admin" 이후의 Endpoint만 작성해주면 되기 때문에
중복 제거와 간편함을 목적으로 분리해보았습니다!

Choose a reason for hiding this comment

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

중복을 제거하는 것이 그렇게까지 큰 편리함을 주지 않을것같아요.
"/admin"은 몇 글자 안되는데, 이 부분을 추출해버리면 오히려 url 전체를 전역 검색해서 찾을 일이 있을 때 잡히지 않아서 큰 불편함을 주죠 :) 프로젝트에 익숙하지 않으면 어디에 있는지 몰라서 한참 찾을 때도 있어요.
각 API 매핑마다 처음부터 path를 다 붙여주는 것은 어떤가요?

Copy link
Author

Choose a reason for hiding this comment

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

오호 엄청 납득이 가는 의견이네요!
바로 변경해보겠습니다!
프로젝트에 익숙하지 않은 사람은 분명 uri를 통해 api 메서드를 검색할 때 꽤나 고생하겠네요
좋은 의견 감사합니다!

import java.util.concurrent.atomic.AtomicLong;

@RestController
@RequestMapping("/reservations")

Choose a reason for hiding this comment

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

Ditto

Copy link
Author

Choose a reason for hiding this comment

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

#10 (comment)

위 리뷰에 답변을 남겨두었습니다:)

}

@Test
void 이단계() {

Choose a reason for hiding this comment

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

무엇을 테스트하고 있는지 적어주는 것은 어떤가요?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

헉 놓쳤던 부분이네요..!
감사합니다! 이 부분은 미션에서 스탭별로 제공하고 있는 테스트 코드이기 때문에 그대로 두고

컨트롤러 별 테스트를 각각의 클래스로 따로 분리해서 작성해보겠습니다 감사합니다:)

@RequestMapping("/reservations")
public class ReservationController {
private final List<Reservation> reservations = Collections.synchronizedList(new ArrayList<>());
private final AtomicLong index = new AtomicLong(1);

Choose a reason for hiding this comment

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

왜 AtomicLong을 사용하셨나요~?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

멀티스레드 환경이기 때문에 AtomicLong을 사용하였습니다!
AtomicLong을 사용함으로써 멀티스레드 환경에서 원자성을 확보하고, 동시성 문제를 방지할 수 있기 때문에
일반 Long이 아닌 AtomicLong을 선택하게 되었습니다!

원자성은 하나의 작업 단위가 완전히 실행되거나, 완전히 실행되지 않았다면 실패해야하는 성질을 의미하는 것으로 알고 있습니다.
만약 2개의 쓰레드가 Long value = 0l; 에 각각 1000번씩 +1을 수행한다면, 일반적인 Long은 원자성을 보장하지 못하기 때문에 데이터 동시 수정 문제가 발생하고, 그 때문에 결과가 2000이 아니게 되는 경우가 발생할 것입니다.

이는 원자성을 보장하지 못하는 동시성 문제이기 때문에 이를 방지하기 위해 AtomicLong을 사용한다고 알고 있고,
그러한 의도로 AtomicLong을 적용해보았는데 맞는 설명과 적용일까요??

정말 원자성을 보장하는 지 궁금해서 직접 테스트도 작성해보았습니다!

Default Long Test

public class AtomicTest {
    static Long value = 0l;

    @DisplayName("AtomicTypeThreadTest")
    @Test
    void atomicTest() {
        Thread t1 = new Thread(() -> {
            for (int i = 0; i < 1000; i++) {
                value++;
            }
        });

        Thread t2 = new Thread(() -> {
            for (int i = 0; i < 1000; i++) {
                value++;
            }
        });

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (Exception e) {
            System.out.println(e.getMessage());
        }

        System.out.println(value);
        assertThat(value).isNotEqualTo(2000);
    }
}
스크린샷 2024-04-17 18 25 41

2000이 되어야하는데 1157로 마무리된 결과


Atomic Long Test

public class AtomicTest {
    static AtomicLong value = new AtomicLong(0);

    @DisplayName("AtomicTypeThreadTest")
    @Test
    void atomicTest() {
        Thread t1 = new Thread(() -> {
            for (int i = 0; i < 1000; i++) {
                value.incrementAndGet();
            }
        });

        Thread t2 = new Thread(() -> {
            for (int i = 0; i < 1000; i++) {
                value.incrementAndGet();
            }
        });

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (Exception e) {
            System.out.println(e.getMessage());
        }

        System.out.println(value.get());
        assertThat(value.get()).isEqualTo(2000);
    }
}
스크린샷 2024-04-17 18 27 33

원자성을 보장하여 정확히 2000 이라는 결과를 흭득

import java.util.List;
import java.util.concurrent.atomic.AtomicLong;

@RestController

Choose a reason for hiding this comment

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

RestController를 사용하셨네요.
왜 Controller가 아니라 RestController를 쓰셨나요?
Controller와는 어떤 차이가 있나요?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

@Controller는 MVC를 위해 사용하는 애노테이션이기 때문에, View를 리턴하는 경우 사용합니다.
하지만 ReservationController 를 통해 전달하고자 하는 것은 View가 아니라 Json 형태의 '데이터'이기 때문에 @RestController 애노테이션을 사용했습니다!

@RestController@Controller와 다르게 Json형태의 데이터를 return하는 이유는
내부적으로 @ResponseBody 애노테이션을 가지고 있기 때문입니다.

@ResposneBody 애노테이션을 사용하게 되면 Response시에 HttpMessageConverter를 상속받은 스프링의 메시지 컨버터 객체가 Response되는 객체의 값을 Json형태로 역직렬화하여 Body에 담아 클라이언트에게 전송하는 것으로 알고 있습니다!

혹시 제가 잘못 알고 있는 부분이 있다면 알려주세요!

<!-- Bootstrap CSS -->
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css">
<link rel="stylesheet" href="/css/style.css">
<meta charset="UTF-8">

Choose a reason for hiding this comment

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

들여쓰기가 다 바뀌어서 약간 고통스럽네요..
indent를 왜 4칸으로 바꾸셨을까요?

Copy link
Author

@PgmJun PgmJun Apr 17, 2024

Choose a reason for hiding this comment

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

죄송합니다 🥲
Command Shift L 로 코드 라인 정리를 했는데 2칸 띄어쓰기가 되어있던 코드였는지
전부 4칸 띄어쓰기로 변경된 것 같습니다..
혹시 확인이 불편하시다면 원래대로 변경해두는게 좋을까요..!?

Choose a reason for hiding this comment

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

어렵지 않으니 한번 바꿔보시면 좋겠네요 ㅎㅎ
html 파일 열고 프로젝트 우측 하단 보시면 기본 설정을 변경하실 수 있어요.
지금은 미션이라 크게 문제가 없지만, indent 규칙을 맞추는게 생각보다 피곤한 문제니 인지하고 계시면 좋을것같아요.

Screen Shot 2024-04-18 at 9 54 48 PM

Copy link
Author

@PgmJun PgmJun Apr 18, 2024

Choose a reason for hiding this comment

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

오호 몰랐던 기능인데 꿀팁 감사합니다!
러너덕과 함께 개발해나가는 코드이니 러너덕이 읽기 편하도록 2칸 띄어쓰기로 다시 변경하겠습니다!

러너덕은 들여쓰기 2칸을 더 선호하시는 편인가요??

@PgmJun
Copy link
Author

PgmJun commented Apr 17, 2024

소중한 리뷰 감사합니다 러너덕!
중요하지만 깊게 생각하지 않고 지나갈 수 있을 법한 내용들에 대해 질문해주셔서 다시 한번 깊게 생각해볼 수 있었어요!

드렸던 질문에 대한 답변도 소중히 듣고 고민해보면서 관련된 부분을 학습하는 중입니다:)

리뷰에 대한 답변 남겨보았는데 확인부탁드리고, 혹시 제가 잘못 알고 있는 부분이 있다면 말씀해주시면 감사하겠습니다~!

Copy link

@Deocksoo Deocksoo left a comment

Choose a reason for hiding this comment

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

안녕하세요 이든,
리뷰 꼼꼼히 확인해주셨네요 👍
url 검색과 관련해서 사소하지만 중요한 것이 하나 있어 요청 남겼어요. 확인 한 번 부탁드려요.

import org.springframework.web.bind.annotation.RequestMapping;

@Controller
@RequestMapping("/admin")

Choose a reason for hiding this comment

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

중복을 제거하는 것이 그렇게까지 큰 편리함을 주지 않을것같아요.
"/admin"은 몇 글자 안되는데, 이 부분을 추출해버리면 오히려 url 전체를 전역 검색해서 찾을 일이 있을 때 잡히지 않아서 큰 불편함을 주죠 :) 프로젝트에 익숙하지 않으면 어디에 있는지 몰라서 한참 찾을 때도 있어요.
각 API 매핑마다 처음부터 path를 다 붙여주는 것은 어떤가요?

<!-- Bootstrap CSS -->
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.5.2/css/bootstrap.min.css">
<link rel="stylesheet" href="/css/style.css">
<meta charset="UTF-8">

Choose a reason for hiding this comment

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

어렵지 않으니 한번 바꿔보시면 좋겠네요 ㅎㅎ
html 파일 열고 프로젝트 우측 하단 보시면 기본 설정을 변경하실 수 있어요.
지금은 미션이라 크게 문제가 없지만, indent 규칙을 맞추는게 생각보다 피곤한 문제니 인지하고 계시면 좋을것같아요.

Screen Shot 2024-04-18 at 9 54 48 PM

Copy link

@Deocksoo Deocksoo left a comment

Choose a reason for hiding this comment

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

리뷰 잘 반영해주셨네요 👍
다음 미션 계속 진행해주시면 될것같아요.

@Deocksoo Deocksoo merged commit 8cb7d92 into woowacourse:pgmjun Apr 18, 2024
@PgmJun PgmJun deleted the step1 branch April 22, 2024 07:14
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.

2 participants