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

[Spring MVC] 백재우 미션 제출합니다. #404

Merged
merged 18 commits into from
Feb 8, 2025

Conversation

jaewoo9797
Copy link

@jaewoo9797 jaewoo9797 commented Feb 6, 2025

안녕하세요 비토 👋

3,4 단계 미션 진행하고 코드 올립니다. toString에 대해서 조금 정리해보았는데, 이미지가 깨진는게 오류가 있나봅니다.
제 깃헙 브랜치 주소 첨부하겠습니다. 잘정리되었다고는 못보지만, 제 생각을 정리했습니다. 하지만 이게 맞는지는 잘 모르겠습니다. 모든 도메인에 재정의해야한다고 하면 조금 부담감이 느껴지네요

toString 정리

체크

  • 병합 브랜치 확인
  • 테스트 코드 확인

고민되는 부분 (해결x) 🤔

  • 에러를 클라이언트에게 반환해줄 때 비즈니스 로직에서 발생하는 일어나는 에러는 어느 레벨까지 정보를 담아서 반환해주는 것이 좋을까?

Copy link
Author

@jaewoo9797 jaewoo9797 left a comment

Choose a reason for hiding this comment

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

리뷰 받기전에 저번 리뷰를 잠시 보면서 조금 더 생각을 정리해보았습니다. 답변할 때 잘 대답해야지 생각을 하다보니 어렵습니다.. 열심히 공부하고 찾아보고 현 상황에서 최선의 답변을 하기위해 노력하겠습니다

@@ -10,7 +10,7 @@ public class WebConfig implements WebMvcConfigurer {
@Override
public void addViewControllers(ViewControllerRegistry registry) {
registry.addViewController("/").setViewName("home");

registry.addViewController("/reservation").setViewName("reservation");
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 6, 2025

Choose a reason for hiding this comment

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

저번 코드리뷰에 답변하지 못했던 부분인데요. 스스로 생각해보니 다른 동료에게 혼동이 올 수 있는 부분이라고 동의합니다.

언제나 코드를 작성할 때 혼자만 보는 것이 아니라는 생각을 하려고 노력하는데, 놓치는 부분이 많은 것 같습니다. 뷰를 반환하는 단순한 컨트롤러를 할당하지 않고 데이터만 주고받는 restController 만 관리하고 싶은 생각이 드는데 이게 적절한 생각인지 잘 모르겠습니다. 리팩터링을 하기 전에 조금 더 가늠해보고 싶습니다.

생각해보겠습니다.

  • 뷰 반환하는 컨트롤러를 만들어서 명확하게 동료들과 혼동없이 코딩
  • 컨트롤러를 뷰 리졸버로만 관리하는 것의 이점이 있는가

Choose a reason for hiding this comment

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

넵, 고민해보시고 나온 답안을 알려주세요! @jaewoo9797 님의 생각을 듣고 저도 제 생각을 적어볼께요!

Copy link
Author

Choose a reason for hiding this comment

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

고민한 결과로는 저는 컨트롤러를 따로 만들지 않을 것 같습니다. 그리고 혼동이 오는 부분을 해결하기 위해서 뷰를 담당하는 서버를 따로 만들고 싶습니다. 데이터를 주고받는 API를 관리하는 현재 서버에서 뷰를 담당하는 서버와 분리를 하고 싶네요.

Choose a reason for hiding this comment

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

저는 컨트롤러를 따로 만들지 않을 것 같습니다.

좋네요! 제 생각은 그냥 뷰 리졸버든 컨트롤러든 뷰를 렌더링하는 로직은 하나의 클래스에 모아두자 입니다! 요즘 웹 프로그래밍에서는 뷰를 렌더링 하는 부분은 백엔드에서 담당하지 않고 대부분 프론트에서 담당합니다. 그래서 지금과 같은 경우는 흔하지 않고 설령있더라도 나중에 프론트에 위임하게 되겠죠. 그러면 하나의 클래스에 모아두는게 이후에 제거하기 용이하기 때문에 하나의 클래스에서 담당할 것 같아요.

추가로 저는 컨트롤러를 선호하는 편입니다! 왜냐하면 뷰 리졸버를 사용하기 위해선 implementation 'org.springframework.boot:spring-boot-starter-thymeleaf' 이 의존성이 추가로 필요한데 컨트롤러를 사용하면 의존성을 추가안해도 작동하는데 굳이...? 싶은 생각이 들거든요.

@RestController
public class ReservationController {

private final Map<Long, Reservation> reservations;
Copy link
Author

Choose a reason for hiding this comment

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

저번 리뷰에서 컨트롤러의 역할에 대해서 이야기를 나누었습니다. 아직 이 부분을 분리하지 않았지만, 데이터를 저장과 수정 조회를 담당하는 책임을 분리해서 다른 레이어에 할당하도록 리팩터링을 진행하겠습니다.

Choose a reason for hiding this comment

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

해당 리뷰를 달려고했는데 이미 인지해주셨네요. 👍

Comment on lines +14 to +17
private final Long id;
private final String name;
private final ReserveDate reserveDate;
private final ReserveTime reserveTime;
Copy link
Author

Choose a reason for hiding this comment

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

비즈니스 요구에 아직 수정이 없으니 모두 final로 불변객체로 만들어주었습니다. 최근 JPA로 애플리케이션 객체와 매핑하는 것을 공부하고 있는데, 뭔가 막막한 부분이 있습니다. 그런데 뭐를 잘 모르는지 메타인지가 좀 안되서 질문도 하기 어렵네요.. 😢

Choose a reason for hiding this comment

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

나중에 공부하실 내용이지만 이후에 실제로 DB를 연결하게 되면 final을 제거하셔야 되긴해요. 그리고 솔직히 지금 코드에서도 final을 안 붙이는게 훨씬 편하긴 합니다. 나중에 저장할때 setId 해버리면 그만이니까요. 하지만 전 가급적이면 정말 라이브러리 상의 문제로 final을 못 쓰는게 아닌한 가능한 붙이는 것을 선호하는 편이긴 해요! final은 @jaewoo9797 님의 생각이 궁금해서 남긴 리뷰였으니 한번 기준을 정해보면 좋을 것 같아요!

Copy link

@unifolio0 unifolio0 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 곰곰! 원래는 내일 하려고했는데 잠깐 코드를 보니 양이 많을 것 같아 빠르게 해치웠어요. 리뷰 양이 꽤 되는데 자유롭게 질문주세요!

build.gradle Outdated
@@ -16,6 +16,9 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter'
implementation 'org.springframework.boot:spring-boot-starter-web'
implementation 'org.springframework.boot:spring-boot-starter-thymeleaf'
implementation 'org.projectlombok:lombok'

Choose a reason for hiding this comment

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

롬복을 추가한 이유는 무엇인가요? 롬복의 장단점을 알려주세요!

Copy link
Author

@jaewoo9797 jaewoo9797 Feb 6, 2025

Choose a reason for hiding this comment

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

반복되는 동일한 코드를 작성하지 않고 어노테이션으로 컴파일 할 때 자동으로 생성해주는 라이브러리라고 생각합니다. 생산성을 향상시켜줍니다. 단점으로는 해당 어노테이션이 코드에서 유용하게 사용되고 있는지 제거될 코드인지 식별하는것이 어렵습니다. 요즈음 생각하는게 명시적인 코드가 존재하지 않 부분은 매우 큰 단점이라고 생각이 듭니다. 또한 제가 자주 겪는 불편함인데요. 프로젝트를 생성할 때 롬복 의존성을 추가하지 않으면 에러가 발생합니다. 그래서 구글링해서 항상 추가 의존성을 복사 붙여넣기 해줍니다.

또 단점이 존재합니다. 러닝커브가 발생한다는 것인데요, 저는 개발을 시작한지 얼마 되지않았을 때 주변에서 롬복을 사용하는 것을 보고 이게 무엇인지 몰랐습니다. 아직도 모르는 롬복 어노테이션 또한 많습니다. 이처럼 해당 라이브러리를 자유롭게 사용하려면 품이 들어가는 것도 단점이라고 생각합니다.

롬복을 추가한 이유는 미션 진행하는데 있어 편리함을 누리려고 입니다..

Choose a reason for hiding this comment

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

롬복이 편리하다는 것에는 저도 동의합니다. 다만 말해주신 것처럼 단점으로는 해당 어노테이션이 코드에서 유용하게 사용되고 있는지 제거될 코드인지 식별하는것이 어렵습니다. 요즈음 생각하는게 명시적인 코드가 존재하지 않 부분은 매우 큰 단점이라고 생각이 듭니다. 이 부분이 가장 큰 단점이라고 생각해요. 이번 스터디에서 기본기를 탄탄하게 만들고 싶다고 하셨는데 그럼 우선 롬복은 제거하는게 어떨까요?

@@ -1,4 +1,4 @@
package roomescape.config;
package roomescape.application.config;

Choose a reason for hiding this comment

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

패키지의 기준이 궁금해지네요. application은 무엇을 의미하나요?

Copy link
Author

Choose a reason for hiding this comment

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

하나의 컴포넌트로 동작하는 단위로 패키징을 쪼개보았습니다. 요즘 MSA를 적용해서 프로젝트를 작은 단위로 쪼개는것에 대해 관심이 있습니다. 그래서 스프링 기능을 사용하지 않는 도메인 패키징을 분리 하였고, application 단에서는 컨트롤러와 서비스 기능을 만들어보려고 설계해보았습니다.

목적은 설명한 대로 배포가능한 단위로 패키징을 한다인데, 적절한 패키징이였는지는 확실하지 않습니다. 분리 된다면 적절히 임포트하고 외부라이브러리를 받아와 사용하는 것처럼 사용해야할 텐데 학습이 필요한 부분인것 같습니다.

Choose a reason for hiding this comment

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

MSA요...? 혹시 소프트웨어 개발 3대 원칙을 아시나요? 만약 진짜 MSA를 의도한 거면 YAGNI에 위반된 코드란 생각이 드네요. 그리고 MSA는 스스로 돌아갈 수 있는 하나의 작은 서비스 단위끼리 나눈 것을 MSA라고 합니다. 지금 패키지를 보면 application안에 있는 클래스만으로 하나의 서비스가 정상적으로 동작되나요?

Copy link
Author

@jaewoo9797 jaewoo9797 Feb 8, 2025

Choose a reason for hiding this comment

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

일단 정정하겠습니다.

제가 분리시켜 하나의 배포할 수 있는 작은 서비스 단위로 나누려고 생각하고 말했던 부분은 repository 레이어입니다. 즉 application과 domain 패키지는 하나의 서비스 단위로 생각하였습니다. 정리하다 보니, MAS 보다는 멀티모듈이라는 느낌이 드네요.

또한 application과 domain을 패키지를 나눈 이유는 핵심적인 비즈니스 도메인에 스프링부트의 mvc 기능이 들어가지 않도록 하기 위해서였습니다.

그리고 YAGNI 처음들어봤습니다. 찾아보고 정리해보겠습니다.

@@ -10,7 +10,7 @@ public class WebConfig implements WebMvcConfigurer {
@Override
public void addViewControllers(ViewControllerRegistry registry) {
registry.addViewController("/").setViewName("home");

registry.addViewController("/reservation").setViewName("reservation");

Choose a reason for hiding this comment

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

넵, 고민해보시고 나온 답안을 알려주세요! @jaewoo9797 님의 생각을 듣고 저도 제 생각을 적어볼께요!

@RestController
public class ReservationController {

private final Map<Long, Reservation> reservations;

Choose a reason for hiding this comment

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

해당 리뷰를 달려고했는데 이미 인지해주셨네요. 👍

public class ReservationController {

private final Map<Long, Reservation> reservations;
private final AtomicLong index;

Choose a reason for hiding this comment

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

index가 과연 컨트롤러의 필드로 적당한지도 고민해보면 좋을것같아요

@Component
public class ReservationConverter {

public Reservation toEntity(Long id, CreateReservationRequestDto requestDto) {

Choose a reason for hiding this comment

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

Entity는 무슨 뜻인가요?

Copy link
Author

Choose a reason for hiding this comment

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

습관입니다.. 생각해보니 Entity 라는 객체가 현재 존재하지도 않고, 네이밍이 의도를 전혀 표현하고 있지 않네요.

리팩터링하게 된다면 해당 부분은 도메인 이름으로 리팩터링을 진행할 것 같습니다. 또한 id 파라미터도 필요하지 않을것 같으니 빼도록 리팩터링 진행할 것 같습니다.


@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public static class FieldError {

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.

hibernate에서 제공하는 Valid 기능에서 발생한 에러에 대한 정보를 담아서 클라이언트의 요청에서 어떤 요청이 잘못되었는지 명시적으로 데이터를 담아서 직관적으로 해결할 수 있도록 하는 데이터의 그릇 역할을 하는 클래스 입니다.

Choose a reason for hiding this comment

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

실제로 사용하는 곳이 있나요?

Copy link
Author

@jaewoo9797 jaewoo9797 Feb 7, 2025

Choose a reason for hiding this comment

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

네 존재합니다。 dto 입력값을 검증하는 부분에서 에러가 생길 경우 이 클래스 필드에 데이터를 담아서 클라이언트에게 에러 메시지를 반환하고 있습니다.

<img src="/temp/images/toString/debug-toString.png" alt="debug-toString-override" style="width:500px; height:auto; object-fit:cover;">
</div>

`toString`을 오버라이딩했을 경우, 디버그 모드에서 객체의 값을 직관적으로 볼 수 있습니다.

Choose a reason for hiding this comment

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

👍 저 이런거 좋아해요! 제 리뷰를 곧이곧대로 안 믿고 자료를 바탕으로 얘기해주는거요.

사실 저도 toString을 디버깅 용도로 자주 씁니다. 그리고 사실 웹 프로그래밍에서 백엔드의 역할은 프론트에 정보를 전달하는 역할이기 때문에 view 용도로 toString이 사용될 일이 없을 겁니다.

- 객체간의 참조가 양방향일 경우 순환 참조의 문제는 어떻게 해결할것인가?

toString 메소드는 뷰 로직 또는 책임인가에 대해서는 잘 모르겠지만, 스스로 생각하기에는 toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다. 또한
디버깅의 목적에 맞도록 효과적으로 정보를 반환하도록 재정의할 때 고려해야합니다.

Choose a reason for hiding this comment

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

toString을 디버깅 용도가 아닌 콘솔 프로그램에서 화면에 출력되는 용도로 쓰게 된다면 view로직이라고 볼 수 있겠죠. 그런데 toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다. 이 부분은 어떤 의미인가요? 혹시 위의 예시처럼 view로직으로 쓰이게 되도 괜찮단 뜻일까요? 만약 그렇다면 mvc를 사용하는 이유가 무엇일까요? domain에 외부 사용자에게 정보를 표현하는 로직이 있는것이 과연 적절할까요? 그러면 dto를 사용하는 이유는 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다.

제가 말한 의도는 객체가 담당하는 데이터의 정보는 해당 객체에서 책임을 가져야한다는 것입니다. 예를 들어보겠습니다.

aggregation 으로 다른 객체를 포함하는 객체가 있다고 가정하겠습니다.

class Foo {
  String name;
  Boo boo;
  
  public String toString() {
    // return name + ", [" + boo.getSome() + ", " + boo.getAnother() + "];
    return name + ", [ " + boo.toString() + "]";
  }
}

객체의 정보를 가지고 오는 getter와 마친가지로 toString을 통해서 객체의 정보를 가지고 오는 것을 의미합니다. 그렇기에 toString은 자신의 정보를 관리하는 책임을 가지는 메서드라는 의미였습니다. 또한 이 포스트에서 말했듯이 디버깅을 하는 목적이지, 콘솔에 출력하기 위해 사용하게 된다면 의도한 부분에서 벗어난다고 생각합니다. 그렇기에 toString은 디버깅을 위한 것이지 뷰 단에서 사용하려는 것이 아닙니다.

그러면 dto를 사용하는 이유는 무엇인가요?
다양한 이유가 존재합니다. dto를 사용하여 깊숙이 존재하는 어플리케이션의 핵심인 비즈니스 도메인과 출력을 담당하는 레이어와의 분리, 객체의 데이터에서 반환하면 안되거나 불필요한 정보를 응답 객체과 차단 등이 존재합니다.

정리하면 toString은 디버깅을 위한 목적으로 사용한다는 것입니다. 이를 뷰 단에서 사용하는 것은 목적에서 벗어나기 때문에 외부 사용자에게 정보를 표현하기 위해 사용되는것은 적절하지 않기에 괜찮지 않다고 생각합니다..

Choose a reason for hiding this comment

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

view용도로만 안 쓴다면 전 상관없다고 생각하긴 합니다!

toString 메소드는 뷰 로직 또는 책임인가에 대해서는 잘 모르겠지만, 스스로 생각하기에는 toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다. 또한
디버깅의 목적에 맞도록 효과적으로 정보를 반환하도록 재정의할 때 고려해야합니다.

그리고 순환참조의 문제로 문제가 발생할 수 있습니다. 이를 해결하는 방법으로는 순환참조 필드를 toString 메서드에서 제외하는 것입니다.

Choose a reason for hiding this comment

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

이 내용은 일단 위의 내용들이 해결된 뒤에 토의해봐요!

@unifolio0
Copy link

에러를 클라이언트에게 반환해줄 때 비즈니스 로직에서 발생하는 일어나는 에러는 어느 레벨까지 정보를 담아서 반환해주는 것이 좋을까?

이 부분은 다음 요청때 조금이라도 생각을 적어주세요!

Copy link
Author

@jaewoo9797 jaewoo9797 left a comment

Choose a reason for hiding this comment

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

비토, 달아주신 리뷰에 대해서 생각하는 이유를 달아놓았습니다.

추가로 ErrorResponse 객체에 대한 답변은 여기에 간단히 해두겠습니다.

ErrorResponse를 현재 공부를 진행하면서 코드를 작성하였습니다. 이 클래스의 코드는 거의 클론에 가깝습니다. 그래서 답변이 조금 어렵습니다. 해당 클래스를 이용해서 어떤 목적을 달성하기 위한 것인지는 인지했지만, 구체적인 코드의 구현은 제 생각을 담지 못하였습니다.

두 가지 말하고 싶은 부분이 있습니다.

첫째로는 파라미터의 final 입니다. 저는 파라미터에 final을 붙이는 것을 선호하지 않습니다. 왜냐하면 가독성이 저하하는 부분과 final을 선언하였다고 하더라도 재할당이 불가능한 것이지 데이터가 변경될 수 있기 때문입니다.

두 번째로는 정적메의 사용입니다.
제가 생각하는 정적메를 사용하는 기준은 객체 생성시에 분리가 될 필요가 있을 때, 캐싱을 해둘 필요가 있을때 입니다. 적절한 예시가 생각나지는 않지만, ErrorResponse에서 사용한 이유는 생성시에 파라미터의 값을 유연하게 받아 객체를 생성하도록 하고 객체 생성 전에 로직을 추가하기 위해서라고 생각합니다.


고민되는 부분 (해결x) 🤔

에러를 클라이언트에게 반환해줄 때 비즈니스 로직에서 발생하는 일어나는 에러는 어느 레벨까지 정보를 담아서 반환해주는 것이 좋을까?

제가 고민한 것을 말해보겠습니다.
세세한 비즈니스 로직에 대한 에러의 정보는 콘솔 로그에 찍는 것이 적절하다고 생각하였습니다. 그렇기에 클라이언트에게는 ErrorCode에서 정한 code 와 메시지만을 전달하는 것이 제 생각입니다. 하지만 비즈니스 로직이 아닌 클라이언트의 입력값에 대한 에러는 잘담아서 어떤 값들이 잘못되었는지는 알려주어야 한다고 생각을 하였습니다.

이 주제에 대해 비토의 생각을 듣고 싶네요!

) {
Reservation createReserve = reservationConverter.toEntity(index.getAndIncrement(), requestDto);
reservations.put(createReserve.getId(), createReserve);
return ResponseEntity
Copy link
Author

Choose a reason for hiding this comment

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

스프링부트는 RestContrller에서 ObjectMapper를 이용해서 객체를 Json 형태로 파싱을 진행하는 것으로 알고있습니다. 하지만 응답을 Json으로 파싱해주는것이지 표준회된 HTTP 응답을 내리는 것은 아닙니다. HTTP는 표준화된 프로토콜로 일관된 방식으로 인터넷이 요청과 응답을 받습니다. 일관된 방식으로 헤더에 정보를 담고 상태코드를 담기위해서 ResponseEntity 객체를 스프링부트에서 제공합니다.

정리하면 일관된 형식의 응답을 클라이언트에게 전송하기 위해서 ResponseEntity를 사용하고 응답하고 싶은 내용인 객체를 Json 형태로 응답하기 위해서 RestController를 사용하였습니다.

private final Long id;
private final String name;
private final ReserveDate reserveDate;
private final ReserveTime reserveTime;
Copy link
Author

Choose a reason for hiding this comment

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

비즈니스 로직이 들어가는 객체를 작게 분리하여 관리하는 자세로 코딩을 임합니다. datetime을 분리하여 생성하였는데, 자바의 날짜와 시간을 관리하는 객체에 대한 이해도 부족이 컸던것 같습니다. 비즈니스 로직을 생각해보니, 날짜와 시간 모두 현재보다는 이후인 시간에만 가능해야합니다. 이에 나누어진 객체를 하나로 합쳐서 관리하는 것이 맞겠다고 생각이 듭니다.

정리하면 큰 이유는 자바의 데이트타임을 관리하는 객체의 이해부족입니다. 두 개를 쪼개서 어떻게 사용해야할지 생각하는 것보다, 빠르게 객체를 나눠서 구현하는 방향으로 잡은건 생각을 덜하고 싶었던 것 같네요. 이참에 어려워하던 시간관련 객체의 사용방법에 대해서 조금 찾아보면서 맛있게 먹어봐야겠네요

import org.springframework.web.bind.annotation.RestControllerAdvice;
import roomescape.common.error.exception.BusinessException;

@Slf4j
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 7, 2025

Choose a reason for hiding this comment

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

롬복에서 제공하는 로깅 기능을 제공하는 어노테이션입니다. 에러를 잡을 때 어느 메서드에서 잡았는지와 어떤 이유로 에러가 발생하였는지 로깅하기 위해서 추가하였습니다. 아직 사용하지는 않았는데, 차후 로깅 목적으로 사용할 것 입니다.

- 객체간의 참조가 양방향일 경우 순환 참조의 문제는 어떻게 해결할것인가?

toString 메소드는 뷰 로직 또는 책임인가에 대해서는 잘 모르겠지만, 스스로 생각하기에는 toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다. 또한
디버깅의 목적에 맞도록 효과적으로 정보를 반환하도록 재정의할 때 고려해야합니다.
Copy link
Author

Choose a reason for hiding this comment

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

toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다.

제가 말한 의도는 객체가 담당하는 데이터의 정보는 해당 객체에서 책임을 가져야한다는 것입니다. 예를 들어보겠습니다.

aggregation 으로 다른 객체를 포함하는 객체가 있다고 가정하겠습니다.

class Foo {
  String name;
  Boo boo;
  
  public String toString() {
    // return name + ", [" + boo.getSome() + ", " + boo.getAnother() + "];
    return name + ", [ " + boo.toString() + "]";
  }
}

객체의 정보를 가지고 오는 getter와 마친가지로 toString을 통해서 객체의 정보를 가지고 오는 것을 의미합니다. 그렇기에 toString은 자신의 정보를 관리하는 책임을 가지는 메서드라는 의미였습니다. 또한 이 포스트에서 말했듯이 디버깅을 하는 목적이지, 콘솔에 출력하기 위해 사용하게 된다면 의도한 부분에서 벗어난다고 생각합니다. 그렇기에 toString은 디버깅을 위한 것이지 뷰 단에서 사용하려는 것이 아닙니다.

그러면 dto를 사용하는 이유는 무엇인가요?
다양한 이유가 존재합니다. dto를 사용하여 깊숙이 존재하는 어플리케이션의 핵심인 비즈니스 도메인과 출력을 담당하는 레이어와의 분리, 객체의 데이터에서 반환하면 안되거나 불필요한 정보를 응답 객체과 차단 등이 존재합니다.

정리하면 toString은 디버깅을 위한 목적으로 사용한다는 것입니다. 이를 뷰 단에서 사용하는 것은 목적에서 벗어나기 때문에 외부 사용자에게 정보를 표현하기 위해 사용되는것은 적절하지 않기에 괜찮지 않다고 생각합니다..

import java.util.Objects;
import roomescape.domain.reservation.exception.ReservationException;

public class ReserveDateAndTimeValidator {
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 7, 2025

Choose a reason for hiding this comment

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

객체를 설계하는 제 생각을 Reservation 클래스에서 말했듯이 최대한 작게 만들기 위해서 분리했습니다. 또한 테스트가 용이해집니다. 해당 검증로직 클래스를 테스트 하여 Reservation 클래스를 테스트 할 때는 비지니스 로직에서 벗어나는 객체를 생성할 수 없다는 것을 신경쓰지 않을 수 있습니다. 또한 Reservation 코드가 번잡해지는 것을 해결하고 비즈니스 요구사항이 변경이 생겼을 때 해당 검증 클래스만 수정을 해주면 되기 때문입니다.

그렇기에 검증로직을 클래스로 분리하여 작성하였습니다.


@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public static class FieldError {
Copy link
Author

Choose a reason for hiding this comment

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

hibernate에서 제공하는 Valid 기능에서 발생한 에러에 대한 정보를 담아서 클라이언트의 요청에서 어떤 요청이 잘못되었는지 명시적으로 데이터를 담아서 직관적으로 해결할 수 있도록 하는 데이터의 그릇 역할을 하는 클래스 입니다.

Copy link

@unifolio0 unifolio0 left a comment

Choose a reason for hiding this comment

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

고민되는 부분 (해결x) 🤔
에러를 클라이언트에게 반환해줄 때 비즈니스 로직에서 발생하는 일어나는 에러는 어느 레벨까지 정보를 담아서 반환해주는 것이 좋을까?
제가 고민한 것을 말해보겠습니다.
세세한 비즈니스 로직에 대한 에러의 정보는 콘솔 로그에 찍는 것이 적절하다고 생각하였습니다. 그렇기에 클라이언트에게는 ErrorCode에서 정한 code 와 메시지만을 전달하는 것이 제 생각입니다. 하지만 비즈니스 로직이 아닌 클라이언트의 입력값에 대한 에러는 잘담아서 어떤 값들이 잘못되었는지는 알려주어야 한다고 생각을 하였습니다.
이 주제에 대해 비토의 생각을 듣고 싶네요!

오, 저도 비슷하게 생각합니다! 우선 당연히 로깅에 들어가는 정보와 클라이언트에게 전송하는 정보는 달라야겠죠. 로깅은 저희 서버 개발자들이 볼 내용이고 클라이언트에 전송하는 정보는 실제 사용자가 볼 수도 있으니까요. 그래서 클라이언트에 전송할 때는 너무 많은 정보를 주지않고 필요한 정보만 주는 것이 좋다고 생각해요. 그 정도는 클라이언트에서 예외가 발생했을때 그것이 서버의 문제라면 가능한 추상화된 정보(서버에 문제가 발생했습니다)로만 전달하는게 맞다고 생각해요. 하지만 클라이언트의 문제라면 클라이언트가 해결할 방법(숫자로만 입력해주세요)까지 전달해주는 편이 좋겠죠.
만약 상태코드로 분리하면 500번대와 400번대의 구분이 되겠네요.

로깅과 같은 경우에도 할 얘기가 많지만 고민점만 드리고 마무리할께요. 지금 단계에선 너무 이른 질문이기도 하니 꼭 답변해주실 필요는 없습니다.

  • 로깅은 과연 무조건 자세하다고 좋은 걸까요? 만약 너무 많은 정보를 담게되면 프로젝트에서 로깅 정보를 기록한다고 할 때 용량으로 인한 문제가 발생할 수도 있겠네요. 과연 로깅은 어느 정도까지가 적당할까요?

build.gradle Outdated
@@ -16,6 +16,9 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter'
implementation 'org.springframework.boot:spring-boot-starter-web'
implementation 'org.springframework.boot:spring-boot-starter-thymeleaf'
implementation 'org.projectlombok:lombok'

Choose a reason for hiding this comment

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

롬복이 편리하다는 것에는 저도 동의합니다. 다만 말해주신 것처럼 단점으로는 해당 어노테이션이 코드에서 유용하게 사용되고 있는지 제거될 코드인지 식별하는것이 어렵습니다. 요즈음 생각하는게 명시적인 코드가 존재하지 않 부분은 매우 큰 단점이라고 생각이 듭니다. 이 부분이 가장 큰 단점이라고 생각해요. 이번 스터디에서 기본기를 탄탄하게 만들고 싶다고 하셨는데 그럼 우선 롬복은 제거하는게 어떨까요?

@@ -1,4 +1,4 @@
package roomescape.config;
package roomescape.application.config;

Choose a reason for hiding this comment

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

MSA요...? 혹시 소프트웨어 개발 3대 원칙을 아시나요? 만약 진짜 MSA를 의도한 거면 YAGNI에 위반된 코드란 생각이 드네요. 그리고 MSA는 스스로 돌아갈 수 있는 하나의 작은 서비스 단위끼리 나눈 것을 MSA라고 합니다. 지금 패키지를 보면 application안에 있는 클래스만으로 하나의 서비스가 정상적으로 동작되나요?

@@ -10,7 +10,7 @@ public class WebConfig implements WebMvcConfigurer {
@Override
public void addViewControllers(ViewControllerRegistry registry) {
registry.addViewController("/").setViewName("home");

registry.addViewController("/reservation").setViewName("reservation");

Choose a reason for hiding this comment

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

저는 컨트롤러를 따로 만들지 않을 것 같습니다.

좋네요! 제 생각은 그냥 뷰 리졸버든 컨트롤러든 뷰를 렌더링하는 로직은 하나의 클래스에 모아두자 입니다! 요즘 웹 프로그래밍에서는 뷰를 렌더링 하는 부분은 백엔드에서 담당하지 않고 대부분 프론트에서 담당합니다. 그래서 지금과 같은 경우는 흔하지 않고 설령있더라도 나중에 프론트에 위임하게 되겠죠. 그러면 하나의 클래스에 모아두는게 이후에 제거하기 용이하기 때문에 하나의 클래스에서 담당할 것 같아요.

추가로 저는 컨트롤러를 선호하는 편입니다! 왜냐하면 뷰 리졸버를 사용하기 위해선 implementation 'org.springframework.boot:spring-boot-starter-thymeleaf' 이 의존성이 추가로 필요한데 컨트롤러를 사용하면 의존성을 추가안해도 작동하는데 굳이...? 싶은 생각이 들거든요.

) {
Reservation createReserve = reservationConverter.toEntity(index.getAndIncrement(), requestDto);
reservations.put(createReserve.getId(), createReserve);
return ResponseEntity

Choose a reason for hiding this comment

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

표준회된 HTTP 응답을 내리는 것은 아닙니다.

이게 무슨 의미인지 더 설명해주세요! 참고로 RestController를 사용해도 충분히 일관되게 전달할 수 있습니다!

import lombok.Builder;
import org.springframework.format.annotation.DateTimeFormat;

@Builder

Choose a reason for hiding this comment

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

빌더를 사용해도 getter를 사용하지 않나요? 그리고 파편화된 생성로직이 걱정되면 Dto에서 Reservation을 파라미터로 받는 생성자나 정팩메를 만들면 해결되지 않나요? 오히려 빌더를 사용하면 dto에 어떤 필드가 존재하는 지, 어떤 필드는 null이 되면 안되는지 파악해야 하는 추가적인 고려사항이 생길 것 같은데 어떻게 생각하시나요?

// then
assertThat(catchThrow).isInstanceOf(BusinessException.class);
}
}

Choose a reason for hiding this comment

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

여기도 하단에 개행 추가해주세요!

@@ -0,0 +1,23 @@
package roomescape.domain.reservation.validator;

import static roomescape.common.error.ErrorCode.*;

Choose a reason for hiding this comment

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

전 개인적으로 이렇게 비즈니스 코드에서 imoort static을 사용할 경우 다른 개발자가 봤을때 이 상수는 현재 클래스에서 정의된 상수라고 판단하기 쉽다고 생각해요. 그래서 막상 찾아봤는데 현재 클래스에 없으면 당황할 것 같아 사용하지 않는 편입니다.

Comment on lines +14 to +17
private final Long id;
private final String name;
private final ReserveDate reserveDate;
private final ReserveTime reserveTime;

Choose a reason for hiding this comment

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

나중에 공부하실 내용이지만 이후에 실제로 DB를 연결하게 되면 final을 제거하셔야 되긴해요. 그리고 솔직히 지금 코드에서도 final을 안 붙이는게 훨씬 편하긴 합니다. 나중에 저장할때 setId 해버리면 그만이니까요. 하지만 전 가급적이면 정말 라이브러리 상의 문제로 final을 못 쓰는게 아닌한 가능한 붙이는 것을 선호하는 편이긴 해요! final은 @jaewoo9797 님의 생각이 궁금해서 남긴 리뷰였으니 한번 기준을 정해보면 좋을 것 같아요!

- 객체간의 참조가 양방향일 경우 순환 참조의 문제는 어떻게 해결할것인가?

toString 메소드는 뷰 로직 또는 책임인가에 대해서는 잘 모르겠지만, 스스로 생각하기에는 toString 을 통해서 객체의 정보를 표현하는 것은 적절한 책임이고 다른 곳에 전가하면 안된다고 생각을 하고 있습니다. 또한
디버깅의 목적에 맞도록 효과적으로 정보를 반환하도록 재정의할 때 고려해야합니다.

Choose a reason for hiding this comment

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

view용도로만 안 쓴다면 전 상관없다고 생각하긴 합니다!


@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public static class FieldError {

Choose a reason for hiding this comment

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

실제로 사용하는 곳이 있나요?

Copy link

@unifolio0 unifolio0 left a comment

Choose a reason for hiding this comment

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

잘못해서 Comment로 남겨서 RC로 바꿀께요...

Copy link
Author

@jaewoo9797 jaewoo9797 left a comment

Choose a reason for hiding this comment

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

비토 안녕하세요. 리뷰 내용에서 수정해야하는 부분 수정완료하였습니다.

  • 코드 컨벤션
  • 불필요한 공백삭제
  • 롬복 의존성 삭제
  • newLine 추가하기

아직 답변 못한 부분이 존재합니다. 해당 부분은 다음 미션을 진행하면서 이전처럼 md 파일에 정리해두겠습니다. 해당 부분은 조금 중요하다고 생각되는 부분이기도 하고 정확하게 정리해둘 필요가 있는 부분이라고 판단되었습니다.

  • HTTP
  • 검증로직 분리하는 것의 생각 정리
  • YANGI 무엇인가
  • converter 가 많아질 경우
  • 빌더 패턴

혹시 제가 놓친 부분이 있다면 답변 달아주세요

;

private final int status;
private final String code;
Copy link
Author

Choose a reason for hiding this comment

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

제가 정의한 에러 코드입니다。 일단 도메인 맨 앞글자를 따서 코드로 관리해보았습니다。 클라이언트에게 응답을 반환할 때 에러 메시지와 코드를 반환해주고 개발자와 클라이언트에서 식별하기 좋아지게 합니다。 또한 일관성이 있다는 부분 때문에 code를 정의했습니다。 잘 모르고 있는 부분이라 일단 앞서 말한 내용대로 생각하고 있습니다。


@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public static class FieldError {
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 7, 2025

Choose a reason for hiding this comment

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

네 존재합니다。 dto 입력값을 검증하는 부분에서 에러가 생길 경우 이 클래스 필드에 데이터를 담아서 클라이언트에게 에러 메시지를 반환하고 있습니다.

Comment on lines 24 to 36
@Override
public Reservation save(Reservation reservation) {
if (Objects.isNull(reservation.getId())) {
reservation = Reservation.builder()
.id(index.getAndIncrement())
.name(reservation.getName())
.reserveDate(reservation.getReserveDate())
.reserveTime(reservation.getReserveTime())
.build();
}
reservations.put(reservation.getId(), reservation);
return reservation;
}
Copy link
Author

Choose a reason for hiding this comment

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

reservation 객체를 생성할 때 자동으로 값을 채워주고 있지 않기 떄문입니다.

객체가 생성될 때 id 값이 null일 수 있어서 저장하는 로직에서 널포인트 예외가 발생할 수 있습니다. 그리고 Map 자료구조의 key , value 로 저장하는 특징으로 저장하는 로직과 업데이트 로직이 매우 유사하게 진행됩니다. 그래서 저장과 업데이트 로직을 하나로 묶고, id 값이 널포인트 에러에서 안전하도록 null 검사를 진행한후 id값을 부여한 후 객체를 저장하도록 했습니다.

import java.util.Optional;
import roomescape.domain.reservation.Reservation;

public interface ReservationRepository {
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 8, 2025

Choose a reason for hiding this comment

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

인터페이스로 만든 이유는 크게 두 가지를 생각하고 만들었습니다.

  1. 도메인 로직이 존재하는 서비스 레이어에서 레포지터리를 의존할 때 구체적인 구현사항은 모르게 하기 위해서 입니다. 클린 아키텍처에 따르면 고수준 컴포넌트는 저수준 컴포넌트를 의존하지 않습니다. 그래서 의존성 역전을 시키기 위해서 인터페이스로 만들었습니다.

  2. 인터페이스르 관리하면 변경에 유연해집니다. 데이터를 저장하는 repository 레이어는 요구사항에 따라 쉽게 변하는 특징이 있습니다. 서비스 레이어에서 구체적인 클래스를 의존할 경우 변경이 발생할 경우 수정해야하는 부분이 변경이 일어난 레포지터리 경계를 넘어 서비스 레이어까지 미치게됩니다. 이에 인터페이스로 만들었고 이후 어떤 데이터베이스로 변경되더라도 변경에는 닫혀있고 확장에는 열려있게 하였습니다.

@@ -1,4 +1,4 @@
package roomescape.config;
package roomescape.application.config;
Copy link
Author

@jaewoo9797 jaewoo9797 Feb 8, 2025

Choose a reason for hiding this comment

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

일단 정정하겠습니다.

제가 분리시켜 하나의 배포할 수 있는 작은 서비스 단위로 나누려고 생각하고 말했던 부분은 repository 레이어입니다. 즉 application과 domain 패키지는 하나의 서비스 단위로 생각하였습니다. 정리하다 보니, MAS 보다는 멀티모듈이라는 느낌이 드네요.

또한 application과 domain을 패키지를 나눈 이유는 핵심적인 비즈니스 도메인에 스프링부트의 mvc 기능이 들어가지 않도록 하기 위해서였습니다.

그리고 YAGNI 처음들어봤습니다. 찾아보고 정리해보겠습니다.

Copy link

@unifolio0 unifolio0 left a comment

Choose a reason for hiding this comment

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

제가 정의한 에러 코드입니다。 일단 도메인 맨 앞글자를 따서 코드로 관리해보았습니다。 클라이언트에게 응답을 반환할 때 에러 메시지와 코드를 반환해주고 개발자와 클라이언트에서 식별하기 좋아지게 합니다。 또한 일관성이 있다는 부분 때문에 code를 정의했습니다。 잘 모르고 있는 부분이라 일단 앞서 말한 내용대로 생각하고 있습니다。

실제로 프로젝트를 하게되면 이렇게 프론트와 주고 받을 부분(api부터 파라미터, 주고받을 데이터, 데이터의 형식 등...)은 하나부터 열까지 프론트와 협의를 거치게 됩니다. 실제로 저는 프로젝트를 진행하며 에러 코드를 사용한 것은 1번밖에 되지 않아요. 만약 프론트가 에러 코드가 필요없다고 했는데 전달하는 것은 불필요한 리소스 낭비겠죠

네 존재합니다。 dto 입력값을 검증하는 부분에서 에러가 생길 경우 이 클래스 필드에 데이터를 담아서 클라이언트에게 에러 메시지를 반환하고 있습니다.

아, 확인했습니다. 대충 무엇을 의도한건지는 알겠어요. 과하다고 생각하긴 하지만 의도는 이해했으니 넘어갈께요.

reservation 객체를 생성할 때 자동으로 값을 채워주고 있지 않기 떄문입니다.
객체가 생성될 때 id 값이 null일 수 있어서 저장하는 로직에서 널포인트 예외가 발생할 수 있습니다. 그리고 Map 자료구조의 key , value 로 저장하는 특징으로 저장하는 로직과 업데이트 로직이 매우 유사하게 진행됩니다. 그래서 저장과 업데이트 로직을 하나로 묶고, id 값이 널포인트 에러에서 안전하도록 null 검사를 진행한후 id값을 부여한 후 객체를 저장하도록 했습니다.

아실지 모르지만 JPA에서는 이미 존재하는 id의 데이터를 저장하면 save가 아닌 update로 처리합니다. 그래서 실제로 이걸 의도하고 구현한 건지 아니면 우연인지 궁금해서 적은 리뷰였어요.

인터페이스로 만든 이유는 크게 두 가지를 생각하고 만들었습니다.
도메인 로직이 존재하는 서비스 레이어에서 레포지터리를 의존할 때 구체적인 구현사항은 모르게 하기 위해서 입니다. 클린 아키텍처에 따르면 고수준 컴포넌트는 저수준 컴포넌트를 의존하지 않습니다. 그래서 의존성 역전을 시키기 위해서 인터페이스로 만들었습니다.
인터페이스르 관리하면 변경에 유연해집니다. 데이터를 저장하는 repository 레이어는 요구사항에 따라 쉽게 변하는 특징이 있습니다. 서비스 레이어에서 구체적인 클래스를 의존할 경우 변경이 발생할 경우 변경이 일어난 레포지터리 경계를 넘어 서비스 레이어까지 미치게됩니다. 이에 인터페이스로 만들었고 이후 어떤 데이터베이스로 변경되더라도 변경에는 닫혀있고 확장에는 열려있게 하였습니다.

repository 레이어가 변할 확률이 높나요? 그러면 Service 레이어도 변경될 확률이 높은데 왜 인터페이스로 처리 안하셨나요?
저는 한 프로젝트에서 DB를 2번 변경한 적이 있습니다.(MySQL -> 파일로 관리 -> MongoDB) 이럴 경우 인터페이스로 관리하건 머건 어짜피 다른 코드까지 갈아엎을 수밖에 없게됩니다.
만약 실제로 DB의 변경에도 비즈니스 코드의 변경을 최소화 하고 싶다면 차라리 멀티모듈을 공부해보는 것을 추천드립니다.
참고로 프로젝트의 규모가 엄청 커지지 않는한 매우 특수한 케이스를 제외하고는 DB가 바뀔일이 거의 없습니다.
멀티모듈 관련 참고 링크

일단 정정하겠습니다.
제가 분리시켜 하나의 배포할 수 있는 작은 서비스 단위로 나누려고 생각하고 말했던 부분은 repository 레이어입니다. 즉 application과 domain 패키지는 하나의 서비스 단위로 생각하였습니다. 정리하다 보니, MAS 보다는 멀티모듈이라는 느낌이 드네요.
또한 application과 domain을 패키지를 나눈 이유는 핵심적인 비즈니스 도메인에 스프링부트의 mvc 기능이 들어가지 않도록 하기 위해서였습니다.
그리고 YAGNI 처음들어봤습니다. 찾아보고 정리해보겠습니다.

솔직히 말하면 무슨 말인지 모르겠어요.

  1. MSA는 저번에 말씀드린것처럼 배포가 가능한 작은 서비스 단위로 나눈 거에요.
    참고 영상
    지금 저희 서비스에서 분리할 수 있는 서비스 단위가 여러개인가요?
  2. 멀티 모듈 느낌도 솔직히 잘 모르겠네요. 위에 repository관련 답변에 달아둔 멀티 모듈 관련 링크를 참고해보면 좋을 것 같아요.

@boorownie boorownie merged commit 75f5933 into next-step:jaewoo9797 Feb 8, 2025
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.

3 participants