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

[4 - 10단계 방탈출 예약 관리] 에버(손채영) 미션 제출합니다. #96

Merged
merged 46 commits into from
Apr 30, 2024

Conversation

helenason
Copy link
Member

@helenason helenason commented Apr 21, 2024

제이미 안녕하세요! 두번째 미션 리뷰 요청 드립니다 :)

먼저, 저번 미션 PR에서 제이미가 머지하며 남겨주었던 코멘트에 대해 고민해보았어요.

1

만약 필드가 동일하다면, DTO를 사용하는 게 더 좋을까요 좋지 않을까요?
DTO 사용 경험이 적었다고 했는데, 이번 기회를 통해 에버만의 기준을 알아가보는 것도 좋겠네요.

도메인 Reservation은 서버간 이동을 하면 안되는 걸까요?

도메인과 DTO의 필드가 동일하더라도 클라이언트에서 요구되는 필드의 종류는 언제든 변경될 수 있기 때문에 도메인과 DTO는 분리하는 것이 낫다고 생각합니다. 마찬가지로, DTO가 아닌 도메인이 서버 사이를 이동하게 되면 클라이언트가 요구하는 필드 종류에 변경 사항이 발생했을 때, 변경 사항을 반영하기 위해 도메인이 변경되어야 하거나 새로운 객체를 생성해야 합니다. 또는 변경 사항을 반영하지 않기 위해 불필요한 데이터가 전달되기도 할 것 같아요!

2

Long을 사용한 이유는 무엇인가요?

해당 질문에 대해서는 저번 단계에서 답을 드렸지만 이번 단계를 구현하면서(데이터베이스를 직접 연결해보면서) 생각이 달라져 제이미에게 말씀드리면서 제이미의 의견 또한 듣고 싶어요!

이전에는, 엔티티의 id가 null이 될 수 없음을 인지하고 있음에도 참조 타입 Long을 사용했습니다. 원시 타입으로 설정할 경우, 정말 부득이하게 id에 null이 입력되었을 때 에러가 반환되는 것이 두려웠습니다. 그에 비해 참조 타입은 에러를 반환하지 않고 프로그램을 진행할 수 있기 때문에, 처리하지 못할 에러는 반환하지 않는 게 낫다고 판단하여 참조 타입을 사용했었습니다.

하지만 조금 더 고민해본 결과, 엔티티의 id(데이터베이스의 id)에 null이 입력된 상황은 비정상적인 상황이 분명하므로, id가 null인 엔티티를 에러 반환 없이 사용하는 것보다는 에러를 반환하여 데이터베이스에 이상이 있음을 알리는 것이 옳다고 생각하게 되었습니다. 따라서 엔티티 Reservation과 ReservationTime의 id를 모두 원시 타입 long으로 수정해주었습니다.

제이미는 이에 대해 어떻게 생각하시는지 궁금해요!


감사합니다. 이번 리뷰도 잘 부탁드립니다! 🙇‍♀️

Copy link

@jamie9504 jamie9504 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 에버 😄 리뷰어 제이미입니다.

코멘트 간략히 남겨두었습니다.

이번 한 주도 화이팅입니다 💪

import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;

@Controller

Choose a reason for hiding this comment

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

컨트롤러에서 특정 UrlPath가 반복되는데, @RequestMapping에 대해 알아볼까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네, @RequestMapping은 메서드 단위에서 특정 엔드포인트를 매핑하는 역할 뿐 아니라, 클래스 단위에서 공유되는 매핑을 표현할 수도 있음을 알게 되었습니다. 관련하여 코드 수정하겠습니다!

Comment on lines 20 to 21
@Autowired
private ReservationService reservationService;

Choose a reason for hiding this comment

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

@Autowired 주입과 final 필드 + 생성자 주입에 대해 알아볼까요?

Copy link
Member Author

@helenason helenason Apr 22, 2024

Choose a reason for hiding this comment

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

제이미의 코멘트를 읽고 관련하여 여러 자료를 찾아보았습니다.

@Autowired는 생성자, 세터, 필드를 통해 의존성을 주입할 때 붙을 수 있습니다. 필드 주입 방식은 필드를 불변으로 설정할 수 없습니다. 따라서 필드를 불변으로 설정하기 위해서는 생성자를 통해 의존성을 주입하는 방식을 선호합니다. 또한, 인스턴스 변수가 하나인 생성자의 경우는 @Autowired가 자동으로 붙는다고 합니다.

따라서 현재 저의 코드에서는 생성자로 의존성을 주입하는 방식이 낫다고 생각합니다! 관련하여 코드 수정하겠습니다.

Comment on lines 17 to 18
@Repository
public class ReservationDao {

Choose a reason for hiding this comment

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

Repository와 Dao의 의미는 각각 무엇일까요?

에버가 만든 친구는 어떤 것에 더 가까운가요?

Copy link
Member Author

@helenason helenason Apr 23, 2024

Choose a reason for hiding this comment

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

하나의 쿼리가 하나의 메서드와 매핑되기 때문에 저의 클래스는 DAO와 가깝다고 생각했습니다.

하지만 조금 더 깊게 생각해보니, 현재 service 계층에서 이루어지고 있는 save -> findById 호출 과정이, repository 계층에서 이루어지면 더욱 자연스럽겠다고 판단하였습니다. 데이터 관련 로직이지만 비즈니스 로직 일부를 포함하고 있기 때문입니다. 따라서 repository와 dao 계층을 따로 두어 repository가 dao보다 조금 더 비즈니스 로직에 가까운 역할을 수행하도록 하였습니다!

Choose a reason for hiding this comment

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

각 계층이 계층별 역할을 해주고 있군요 👍
계층을 나눌 때는 그 계층이 꼭 필요한지에 대한 고민도 함께 해보면 좋습니다.

많은 역할을 하지 않는다면, 해당 계층에서 조금 더 많은 일을 해도 되는게 아닌지
많은 역할을 하지 않더라도, 해당 계층에선 명확히 해당 계층의 일을 해야 하는지

똑같지만 반대의 상황에서도 항상 고려해보면서 에버만의 기준을 세워보는 것도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

네, 사실 지금 상황에서는 save -> findById 호출 흐름을 한 번에 호출할 수 있다는 이점 외에는 두 계층을 분리한 장점이 추가로 보이지 않습니다. 하나의 repository가 여러 dao에 의존해야 하는 상황이 발생하거나, 데이터베이스의 여러 테이블에 걸쳐 작업이 수행되어야 하는 경우에 repository와 dao 계층을 나누도록 하겠습니다! 지금은 다시 생각해보니 두 계층 간의 역할 차이가 명확하지 않은 것 같아요 ㅎㅎ 😂

Comment on lines 26 to 34
RowMapper<Reservation> rowMapper = (resultSet, rowNum) -> new Reservation(
resultSet.getLong("id"),
resultSet.getString("name"),
resultSet.getDate("date").toLocalDate(),
new ReservationTime(
resultSet.getLong("time_id"),
resultSet.getTime("time_value").toLocalTime()
)
);

Choose a reason for hiding this comment

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

RowMapper는 매번 생성해줘야할까요?

아래 findById와도 중복되는 것 같네요!

Copy link
Member Author

@helenason helenason Apr 22, 2024

Choose a reason for hiding this comment

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

그렇네요. 하나의 DAO 클래스 당 하나의 RowMapper가 필요하다고 생각되어 필드로 선언해주었습니다!

}

public long save(ReservationRequestDto reservationDto) {
String sql = "INSERT INTO reservation(name, date, time_id) VALUES (?, ?, ?)";

Choose a reason for hiding this comment

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

SimpleJdbcInsert에 대해 알아봐도 좋겠네요!


public class Reservation {

private final Long id;
private final long id;

Choose a reason for hiding this comment

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

해당 질문에 대해서는 저번 단계에서 답을 드렸지만 이번 단계를 구현하면서(데이터베이스를 직접 연결해보면서) 생각이 달라져 제이미에게 말씀드리면서 제이미의 의견 또한 듣고 싶어요!

이전에는, 엔티티의 id가 null이 될 수 없음을 인지하고 있음에도 참조 타입 Long을 사용했습니다. 원시 타입으로 설정할 경우, 정말 부득이하게 id에 null이 입력되었을 때 에러가 반환되는 것이 두려웠습니다. 그에 비해 참조 타입은 에러를 반환하지 않고 프로그램을 진행할 수 있기 때문에, 처리하지 못할 에러는 반환하지 않는 게 낫다고 판단하여 참조 타입을 사용했었습니다.

하지만 조금 더 고민해본 결과, 엔티티의 id(데이터베이스의 id)에 null이 입력된 상황은 비정상적인 상황이 분명하므로, id가 null인 엔티티를 에러 반환 없이 사용하는 것보다는 에러를 반환하여 데이터베이스에 이상이 있음을 알리는 것이 옳다고 생각하게 되었습니다. 따라서 엔티티 Reservation과 ReservationTime의 id를 모두 원시 타입 long으로 수정해주었습니다.

제이미는 이에 대해 어떻게 생각하시는지 궁금해요!

long 타입을 사용할 때 주의해야 할 부분은 null이 입력되었을 때 long 타입의 기본값인 0이 들어오는 경우입니다.
들어오지 않음과 0은 명백히 다른 의미이니까요!

라이브러리나 프레임워크를 사용하는 경우, long 타입 필드에 무언가를 주입할 때 null인 경우 기본값을 주입하는 경우가 있어서 이 부분을 주의해야 합니다.

그렇지 않고 모든 부분을 제어할 수 있는 상황이라면, 에버의 취향대로 구현하면 되죠 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

네, 제이미의 코멘트를 참고하여 엔티티 클래스에 id의 범위를 검증하는 로직을 추가하였습니다!

Choose a reason for hiding this comment

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

ID를 항상 1 이상으로 사용해야겠군요!

Comment on lines 25 to 26
public ReservationResponseDto addReservation(ReservationRequestDto reservationDto) {
long id = reservationDao.save(reservationDto);

Choose a reason for hiding this comment

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

도메인과 DTO의 필드가 동일하더라도 클라이언트에서 요구되는 필드의 종류는 언제든 변경될 수 있기 때문에 도메인과 DTO는 분리하는 것이 낫다고 생각합니다. 마찬가지로, DTO가 아닌 도메인이 서버 사이를 이동하게 되면 클라이언트가 요구하는 필드 종류에 변경 사항이 발생했을 때, 변경 사항을 반영하기 위해 도메인이 변경되어야 하거나 새로운 객체를 생성해야 합니다. 또는 변경 사항을 반영하지 않기 위해 불필요한 데이터가 전달되기도 할 것 같아요!

에버가 DTO로 분리한 이유를 잘 알았습니다 👍

현재 코드에선 DTO가 DAO까지 전달되고 있는데요. 그렇다면 클라이언트에서 보낸 객체가 DAO까지 영향을 주는 것인데 이 부분은 의도한 부분인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네 의도한 부분이었습니다. 하나의 클래스를 계속 사용하고 싶었던 바람이 있었습니다 😅

하지만 이러한 경우, 클라이언트로부터 받는 데이터가 변경될 경우 DAO의 코드 및 쿼리까지 변경되어야하는 상황이 발생하겠네요. 변경 사항이 전이되는 상황을 막기 위해 DAO로 전달되는 객체를 다른 DTO로 감싸겠습니다!

Choose a reason for hiding this comment

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

네 의도한 부분이었습니다. 하나의 클래스를 계속 사용하고 싶었던 바람이 있었습니다 😅

하지만 이러한 경우, 클라이언트로부터 받는 데이터가 변경될 경우 DAO의 코드 및 쿼리까지 변경되어야하는 상황이 발생하겠네요. 변경 사항이 전이되는 상황을 막기 위해 DAO로 전달되는 객체를 다른 DTO로 감싸겠습니다!

Entity가 먼저 존재했고, 이를 보내주고 받기 위해 DTO를 사용할지는 선택입니다.
이를 Controller에서 외부에 보내주고 받을 때 사용하는지, Repository에서 외부로 보내주고 받을 때 사용하는지도 선택이죠.
만약 두 DTO의 필드가 같다면, 같은 객체로 사용해도 무관합니다.

나눠서 사용할지, 같이 사용할지, Entity만 사용할지 등은 개인과 협업자들의 취향과 선택에 따라서 달라질 수 있습니다.

DAO의 save 메서드 파라미터로 DTO를 전달할지, 엔티티를 전달할지에 대해 고민이 있었습니다.

save 메서드의 파라미터로 들어가는 객체는 id가 null이어야 합니다. 엔티티는 데이터베이스의 데이터를 나타내는 객체라고 생각하는데, 이러한 엔티티의 id가 null인 상황은 불완전하다고 판단하였습니다. 따라서 이를 엔티티가 아니라고 생각하고 파라미터로 들어가는 객체를 DTO로 설정하였습니다. 이에 대해 제이미는 어떻게 생각하시는지 궁금합니다!

위 질의 부분까지 포함하여 코멘트 남겨보자면,

에버가 생각하는 Entity가 위와 같다면 파라미터로 들어가는 객체를 DTO로 설정하는 것이 좋다고 생각합니다.
만약 파라미터로 들어가는 객체닥 Entity라면 에버가 생각하는 Entity에 대한 기준을 조금 느슨하게 잡을 필요가 있겠죠 😃

정해진 것은 없지만, 스스로가 정한 기준에서 적합한 방향으로 가면 된다고 생각합니다.

단, DTO를 사용하는 경우 Entity에 검증로직이 추가된다면 관리를 어떻게 하는 게 좋을지 등등에 대한 고려를 해보면 좋겠죠 😉
원리원칙대로 갈지, 편의성을 택할지도 본인의 취향과 선택이라고 생각합니다.

에버만의 기준을 만들어보아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

DTO를 사용하는 경우 Entity에 검증로직이 추가된다면 관리를 어떻게 하는 게 좋을지

DAO에서 DTO를 entity로 변환하는 과정에서, DTO에 유효하지 않은 값이 담겨 있어도 entity로의 변환 과정에서 검증을 통해 유효하지 않음이 입증되므로, DTO를 사용함으로 인해 추가될 부분은 없다고 판단했습니다!

정해진 것은 없고, 본인의 기준에 적합한 방향으로 가면 된다는 코멘트 새겨듣겠습니다. 알고 있으면서도 계속 정답을 찾으려고 하는 습관이 나오네요.. 감사합니다 😃

@helenason
Copy link
Member Author

안녕하세요 제이미!

제이미의 코멘트 관련하여 열심히 고민해보고 코드에 반영해두었습니다.

추가로, 한 가지 질문을 드리고 싶습니다.

DAO의 save 메서드 파라미터로 DTO를 전달할지, 엔티티를 전달할지에 대해 고민이 있었습니다.

save 메서드의 파라미터로 들어가는 객체는 id가 null이어야 합니다. 엔티티는 데이터베이스의 데이터를 나타내는 객체라고 생각하는데, 이러한 엔티티의 id가 null인 상황은 불완전하다고 판단하였습니다. 따라서 이를 엔티티가 아니라고 생각하고 파라미터로 들어가는 객체를 DTO로 설정하였습니다. 이에 대해 제이미는 어떻게 생각하시는지 궁금합니다!

이번 리뷰도 잘 부탁드립니다! 편하게 리뷰해주세요! ☺️

Copy link

@jamie9504 jamie9504 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 에버 😄 리뷰어 제이미입니다!

코멘트 조금 더 남겨두었어요!

이슈 없다면 다음 리뷰 요청시 Approve & Merge 예정입니다.
오늘도 좋은 하루 보내세요~!

@RequestMapping("/admin")
public class AdminController {

@GetMapping("")

Choose a reason for hiding this comment

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

이렇게 되는 경우 ("")을 생략할 수 있을거에요!

Comment on lines 17 to 18
@Repository
public class ReservationDao {

Choose a reason for hiding this comment

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

각 계층이 계층별 역할을 해주고 있군요 👍
계층을 나눌 때는 그 계층이 꼭 필요한지에 대한 고민도 함께 해보면 좋습니다.

많은 역할을 하지 않는다면, 해당 계층에서 조금 더 많은 일을 해도 되는게 아닌지
많은 역할을 하지 않더라도, 해당 계층에선 명확히 해당 계층의 일을 해야 하는지

똑같지만 반대의 상황에서도 항상 고려해보면서 에버만의 기준을 세워보는 것도 좋을 것 같아요!

Comment on lines 25 to 26
public ReservationResponseDto addReservation(ReservationRequestDto reservationDto) {
long id = reservationDao.save(reservationDto);

Choose a reason for hiding this comment

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

네 의도한 부분이었습니다. 하나의 클래스를 계속 사용하고 싶었던 바람이 있었습니다 😅

하지만 이러한 경우, 클라이언트로부터 받는 데이터가 변경될 경우 DAO의 코드 및 쿼리까지 변경되어야하는 상황이 발생하겠네요. 변경 사항이 전이되는 상황을 막기 위해 DAO로 전달되는 객체를 다른 DTO로 감싸겠습니다!

Entity가 먼저 존재했고, 이를 보내주고 받기 위해 DTO를 사용할지는 선택입니다.
이를 Controller에서 외부에 보내주고 받을 때 사용하는지, Repository에서 외부로 보내주고 받을 때 사용하는지도 선택이죠.
만약 두 DTO의 필드가 같다면, 같은 객체로 사용해도 무관합니다.

나눠서 사용할지, 같이 사용할지, Entity만 사용할지 등은 개인과 협업자들의 취향과 선택에 따라서 달라질 수 있습니다.

DAO의 save 메서드 파라미터로 DTO를 전달할지, 엔티티를 전달할지에 대해 고민이 있었습니다.

save 메서드의 파라미터로 들어가는 객체는 id가 null이어야 합니다. 엔티티는 데이터베이스의 데이터를 나타내는 객체라고 생각하는데, 이러한 엔티티의 id가 null인 상황은 불완전하다고 판단하였습니다. 따라서 이를 엔티티가 아니라고 생각하고 파라미터로 들어가는 객체를 DTO로 설정하였습니다. 이에 대해 제이미는 어떻게 생각하시는지 궁금합니다!

위 질의 부분까지 포함하여 코멘트 남겨보자면,

에버가 생각하는 Entity가 위와 같다면 파라미터로 들어가는 객체를 DTO로 설정하는 것이 좋다고 생각합니다.
만약 파라미터로 들어가는 객체닥 Entity라면 에버가 생각하는 Entity에 대한 기준을 조금 느슨하게 잡을 필요가 있겠죠 😃

정해진 것은 없지만, 스스로가 정한 기준에서 적합한 방향으로 가면 된다고 생각합니다.

단, DTO를 사용하는 경우 Entity에 검증로직이 추가된다면 관리를 어떻게 하는 게 좋을지 등등에 대한 고려를 해보면 좋겠죠 😉
원리원칙대로 갈지, 편의성을 택할지도 본인의 취향과 선택이라고 생각합니다.

에버만의 기준을 만들어보아요!


private final ReservationService reservationService;

@Autowired

Choose a reason for hiding this comment

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

해당 애너테이션의 생략 가능 여부에 대해서도 알아보세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

네! 생성자가 하나인 경우 @Autowired를 명시하지 않아도 자동으로 적용된다고 알고 있습니다. 그렇지만 저는 해당 생성자를 통해 의존 관계가 자동으로 설정됨을 어노테이션을 통해 드러내고 싶어 생략하지 않았습니다 😁

Choose a reason for hiding this comment

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

Spring에서 생성주기를 관리하는 경우, 생략하더라도 묵시적으로 의존 관계가 자동적으로 설정된다고 알 수 있지만
스프링에 익숙해질 때까지는 달아줘도 좋겠네요!

Comment on lines 35 to 47
@PostMapping("")
public ResponseEntity<ReservationResponseDto> addReservation(@RequestBody ReservationRequestDto requestBody) {
ReservationResponseDto responseBody = reservationService.addReservation(requestBody);
return ResponseEntity
.created(URI.create("/reservations/" + responseBody.getId()))
.body(responseBody);
}

@DeleteMapping("/{id}")
public ResponseEntity<Void> deleteReservation(@PathVariable("id") long id) {
reservationService.deleteReservation(id);
return ResponseEntity.noContent().build();
}

Choose a reason for hiding this comment

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

RequestBody가 미흡하거나 (Null이 들어온다던가...), 없는 ID가 들어온다면
어떠한 응답을 내려주고 있을까요?

HTTP Status Code에 대해 알아보고, 에버가 의도한 코드를 내려주고 있는지 고민해보세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

이번 단계에서는 happy case만 고려하라는 요구 사항이 있어서, 1~3단계에서 예외 케이스를 처리 했었지만 현재는 해당 코드를 삭제해 둔 상태입니다! 요청 바디가 미흡한 경우, 요청된 id에 해당하는 데이터가 없는 경우 등 제이미가 말씀 주신 모든 예외 케이스에 대해 다음 단계에서 처리해보겠습니다! 감사해요 🥹


public class Reservation {

private final Long id;
private final long id;

Choose a reason for hiding this comment

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

ID를 항상 1 이상으로 사용해야겠군요!

@helenason
Copy link
Member Author

안녕하세요!

제이미의 코멘트 반영하여 코드 수정해보았어요.

추가로, 10단계 미션도 구현해보았습니다!

이번 리뷰도 잘 부탁드립니다 😃

Copy link

@jamie9504 jamie9504 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 에버 😄 리뷰어 제이미입니다.

가벼운 코멘트만 남겨두었습니다.

다음 리뷰요청때는 이슈 없다면 Approve & Merge 예정입니다.
주말 푹 쉬세요 😉

Comment on lines 8 to 9
@SpringBootApplication
public class RoomEscapeConsoleApplication {

Choose a reason for hiding this comment

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

Console을 제공하는 경우에도 SpringBoot여야 할까요?

만약, 콘솔 UI를 제공하고 나서 추후 Web UI를 제거한다고 가정했을 때
Web Depedency를 모두 삭제하더라도 콘솔 UI는 동작하나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

콘솔 뷰를 사용하는 어플리케이션의 경우에도 @Autowired 어노테이션을 사용해 의존성 주입을 간편하게 하고 싶은 바람이 있었습니다. 훨씬 편리하더라구요.. 하지만 제이미의 코멘트를 읽고 생각해보니, 웹에 의존하는 스프링을 삭제한 후에도 콘솔 UI는 문제없이 동작해야겠네요. 그렇기에 스프링을 사용하지 않고 의존성을 주입하는 과정이 필수겠어요. 콘솔 뷰를 제공하는 부분은 스프링과 분리시키도록 하겠습니다!

Choose a reason for hiding this comment

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

편리한 건 좋지만, 사용하지 않아야 할 부분에 의존해버린다면
뭔가 나중에 유지보수할 때 관리를 하기 어려워지는 점이 있답니다 😃

@RequestMapping("/admin")
public class AdminController {

@GetMapping()

Choose a reason for hiding this comment

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

()도 생략할 수 있을거에요!

import roomescape.view.ConsoleView;

@Component
public abstract class Command {

Choose a reason for hiding this comment

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

Console은 Command 패턴을 이용했는데요.
Web은 이용하지 않았죠? 어떤 기준으로 방식을 선택하고 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Command 패턴을 도입한 이유는 ConsoleController의 run 메서드가 길어지는 상황을 방지하고, 명령어의 동작을 캡슐화하여 controller가 명령어의 내부 코드를 알지 못한 채 실행되게 하고 싶었습니다.

이를 웹의 경우와 비교한다면, 하나의 API 당 하나의 Command 클래스를 도입했다고 볼 수 있을 것 같아요!

Choose a reason for hiding this comment

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

사실상 Web의 Controller의 메서드가 하나의 Command 클래스의 API와 매핑되는 구조인 것인데,
Controller의 역할이 조금 다른 것 같기도 하네요 😃

Web을 생각해봤을 땐, Console Controller에서 내부 코드를 몰라야할지도 고민해보면 재밌을 것 같아요.


private final ReservationService reservationService;

@Autowired

Choose a reason for hiding this comment

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

Spring에서 생성주기를 관리하는 경우, 생략하더라도 묵시적으로 의존 관계가 자동적으로 설정된다고 알 수 있지만
스프링에 익숙해질 때까지는 달아줘도 좋겠네요!

@helenason helenason changed the title [4 - 9단계 방탈출 예약 관리] 에버(손채영) 미션 제출합니다. [4 - 10단계 방탈출 예약 관리] 에버(손채영) 미션 제출합니다. Apr 28, 2024
@helenason
Copy link
Member Author

helenason commented Apr 29, 2024

안녕하세요 제이미! 코멘트 반영하여 코드 수정하였습니다.

이번 리뷰 반영 과정에서는 웹 뷰를 사용하는 로직과 콘솔 뷰를 사용하는 로직을 분리하기 위해 노력했어요!

LMS에 추가된 요구사항인 콘솔 UI를 사용해 생성되는 데이터는 데이터베이스가 아닌 메모리에 저장합니다. 또한 적용해두었습니다.

이번 리뷰도 잘 부탁드립니다! 😁

Copy link

@jamie9504 jamie9504 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 에버 😄 리뷰어 제이미입니다.

지난번 남겼던 코멘트대로 이슈 없어서 이만 Approve & Merge 합니다.

코멘트 조금 남겨두었으니 가볍게 확인해보세요!
새로운 미션도 화이팅입니다 💪

Comment on lines 8 to 9
@SpringBootApplication
public class RoomEscapeConsoleApplication {

Choose a reason for hiding this comment

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

편리한 건 좋지만, 사용하지 않아야 할 부분에 의존해버린다면
뭔가 나중에 유지보수할 때 관리를 하기 어려워지는 점이 있답니다 😃

Comment on lines +15 to +20
ConsoleView view = new ConsoleView();
ReservationDao reservationDao = new ReservationDaoMemory();
ReservationTimeDao reservationTimeDao = new ReservationTimeDaoMemory();
ReservationService reservationService = new ReservationService(reservationDao, reservationTimeDao);
ReservationTimeService reservationTimeService = new ReservationTimeService(reservationTimeDao);
ConsoleController controller = new ConsoleController(view, reservationService, reservationTimeService);

Choose a reason for hiding this comment

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

코드를 보았을 때, 꼭 생성자로 넣어줘야 하는 부분들인건지 검토해봐도 좋을 것 같아요 😃

}
}

private Command createCommand(Class<? extends Command> commandType) {

Choose a reason for hiding this comment

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

commandType을 단순 분기용으로 사용할 것이라면, 꼭 Class<? extends Command>로 받을 필요가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저의 첫 의도는, 현재의 InputCommandMapper 클래스 findCommand 메서드가 Command 타입의 객체를 생성하여 반환하는 것이었습니다. 하지만 Command 객체를 생성하기 위해서는 ConsoleView, ReservationService, ReservationTimeService 객체를 새로 생성해야 합니다. 함수형 인터페이스를 사용하더라도 사용되는 파라미터의 타입을 통일시키기 어려운 상황이었습니다. 따라서, Mapper 클래스는 CommandType만을 반환하도록 하였고 뷰와 서비스 클래스를 필드로 갖고 있는 ConsoleController 클래스가 Command 객체를 생성하도록 하였습니다.

다른 방법으로는, 기본 클래스 InputCommandMapper가 뷰와 서비스를 필드로 갖도록 하여, 입력값과 Command 객체를 매핑하는 방식이 있습니다. 하지만 이 경우 컨트롤러의 역할을 하는 Mapper 클래스가 뷰 로직 일부를 알게 됩니다. 그럼에도 현재는 해당 방식이 최선이라고 생각되기 때문에 이 방향으로 코드 수정해보겠습니다!

import roomescape.view.ConsoleView;

@Component
public abstract class Command {

Choose a reason for hiding this comment

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

사실상 Web의 Controller의 메서드가 하나의 Command 클래스의 API와 매핑되는 구조인 것인데,
Controller의 역할이 조금 다른 것 같기도 하네요 😃

Web을 생각해봤을 땐, Console Controller에서 내부 코드를 몰라야할지도 고민해보면 재밌을 것 같아요.

public class ReservationTimeDaoMemory implements ReservationTimeDao {

private final Map<Long, ReservationTime> reservationTimes = new LinkedHashMap<>();
private long key = 1;

Choose a reason for hiding this comment

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

long을 쓴다면 어떤 문제가 있을 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

여러 스레드가 공유된 자원에 동시에 접근하는 경우 원자성이 보장되지 않아 데이터의 무결성과 일관성이 보장되지 않는 문제가 있다고 합니다!

@jamie9504 jamie9504 merged commit 4d05a13 into woowacourse:helenason Apr 30, 2024
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