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 - 3단계 방탈출 사용자 예약] 카피(김상혁) 미션 제출합니다. #26

Merged
merged 65 commits into from
May 6, 2024

Conversation

tkdgur0906
Copy link
Member

@tkdgur0906 tkdgur0906 commented May 2, 2024

안녕하세요 안돌, 카피입니다!!

이전 미션의 제 코드를 사용해서 페어와 함께 미션을 수행했습니다

이번 단계 변경사항은 링크에서 보실 수 있습니다.

이전의 웹 개발 경험

스프링을 통한 프로젝트 개발 경험이 있습니다.
AWS를 통해 배포해본 적이 있습니다.

집중한 부분

  • 시간이 넉넉하지 않아 구현하는 것 자체에 집중을 했습니다.
  • 이전 미션에서 이런저런 구조를 이미 만들어 놓아서 설계적인 측면에서는 고민을 따로 하지 않았습니다.

잘 모르겠는 부분

  • E2E, 인수 테스트 같은 개념들이 아직 학습이 되지 않았습니다. 그래서 controller, service에 관한 테스트를 작성할 때 주먹구구식으로 만든 것 같습니다.

더 알고싶은 부분

  • 여러 종류의 테스트 작성 방법과 기술들에 대해 학습해보고 싶습니다.
  • 커스텀 예외를 사용한 크루들도 있는데 장단점에 대한 고민을 해보고 싶습니다.

질문

data.sql을 모든 테스트가 의존하는 문제

data.sql를 사용해서 테스트 코드의 기본 데이터를 생성해주고 있습니다.
그래서 controller, service의 모든 테스트가 해당 sql을 의존해서 테스트를 수행하고 있습니다.
data.sql이 변경되면 테스트에 문제가 생길수 있는 문제가 있는데, 이렇게 의존하는 것에 대을 해결할 수 있는 키워드나 안돌의 의견이 궁금합니다!

service를 crud로 분리하는 구조

기존에 service 하나에 모든 서비스 로직이 있는 것보다는 생성, 조회, 삭제에 대한 기능으로 서비스를 나눠 분류하는 것이 유지보수, 가독성 측면에서 좋다고 생각하여 분류했습니다.
하지만 페어가 준 의견은 그렇게 분류하면 서비스가 db 로직에 너무 의존해서 분류되는 것이 아닌가라는 의견을 주었습니다.
그것 보다는 차라리 business 로직에 따라서 service를 분류하는게 좋지 않을까?라는 말을 해줬습니다.
그래서 현재 저의 생각은 service하나에 모든 로직이 담겨져 있는 것은 지양하자는 것이지만, service를 어떻게 나눌 수 있을까에 대한 생각은 정리되지 않았습니다.
이에 대해서 주실수 있는 의견이 있는지 궁금합니다!

tkdgur0906 and others added 30 commits April 30, 2024 14:27
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
Co-authored-by: nak-honest <e5slnh9876@gmail.com>
@tkdgur0906
Copy link
Member Author

tkdgur0906 commented May 6, 2024

e2e, 인수 테스트에 관심을 갖게 된 이유가 궁금하네요

인수 테스트는 이번 미션을 진행하면서 접한 용어이고, e2e는 커리큘럼에 적혀있어서 알게되었습니다!
지금은 이건 뭐지? 공부해볼까? 하는 정도의 관심입니다!

현재 어노테이션 검증에 대한 테스트를 작성하지 않았는데, 추후에 테스트 방법에 대해 공부해보고 적용해볼 예정입니다.

더 알고 싶은 부분

  • 이번 미션을 진행하면서 예외 처리에 대한 저의 생각이 모호하다는 느낌을 받았는데, 이번 기회에 저의 생각을 정리할 수 있으면 좋겠습니다!

reservationStatus.getReservationStatus()
.keySet()
.stream()
.map(reservationTime -> new ReservationStatusResponse(
Copy link

Choose a reason for hiding this comment

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

만약 ReservationStatusResponse가 List<ReservationStatus> 를 필드로 갖는다면 이 스트림처리가 Response내로 옮겨갈수 있겠네요.

Copy link

Choose a reason for hiding this comment

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

List<T>로 응답하는것을 자제하라는 말이 있기도 합니다. 페이징처리가 필요하게 되면 api가 바뀌어야 하거든요.

Copy link
Member Author

Choose a reason for hiding this comment

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

둘다 동의하는 부분입니다. 그래서 적용하려 해봤는데, 기존에 정의되어있는 js를 수정해야되서 소요가 클 것 같아 추후에 리팩토링 시도해보도록 하겠습니다!

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

@Controller
public class ViewController {
Copy link

Choose a reason for hiding this comment

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

view 라고 하는 리소스는 없는것 같네요. 특정 리소스들을 api / view로 서빙할지 선택하는 것이지 view를 제어하는건 아닌것 같아요.

db와 관련한 로직을 dbService 라는곳에서 처리하도록 하나요?

this(null, name, date, reservationTime, theme);
}

public boolean isSameReservation(Long id) {
Copy link

Choose a reason for hiding this comment

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

별로 좋지 않은 방식이라고 생각해요. id가 같다고 하면 같은 entity일순 있겠지만, entity가 update될 가능성이 있다고 가정하면 id가 같다고 해서("동일"하다고 해서) "동등" 하지는 않는것 같아요.

jdbcTemplate.update(sql, id);
}

public boolean existsByReservationTimeId(Long reservationTimeId) {
Copy link

Choose a reason for hiding this comment

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

파라미터가 long 타입이라면 null 가능성을 원천 차단할수 있겠네요


@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)
public class ReservationTimeControllerTest {
Copy link

Choose a reason for hiding this comment

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

이 테스트는 e2e or acceptanceTest라고 볼수 있겠네요.

내부 구현에 대한 제어 없이(블랙박스) 드러낸 api만으로 기능을 검증했어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 느낌이 e2e, 인수 테스트 라고 볼 수 있군요!
학습 더하고 슬랙으로 토픽 질문 가겠습니다!

date DATE NOT NULL,
reservation_time_id BIGINT,
theme_id BIGINT,
UNIQUE (date, reservation_time_id, theme_id),-- 컬럼 추가
Copy link

Choose a reason for hiding this comment

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

모두 같은 데이터가 있을 수 있다고 보기 때문에

그런가요? 어떤 경우에 같은 데이터가 있을수 있나요?

unique를 물어본 것은 date, reservation_timd_id, theme_id 순으로 구성한 이유를 물어본것이었어요.

Copy link

@andole98 andole98 left a comment

Choose a reason for hiding this comment

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

잘 구현해 주셨네요~ 특히 테스트를 잘 보강해 주신것 같아요.

몇가지 코멘트 남겼습니다. 필요하다면 채널 or pr 스레드에서 이야기해보시죠

@andole98 andole98 merged commit 622dc77 into woowacourse:tkdgur0906 May 6, 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