[BE] 지출로그 추가 기능 구현

[Arachneee]

issue

• close [BE] 지출 로그 추가 기능 구현 #16

구현 사항

지출 로그 추가 기능을 구현했습니다.

중점적으로 리뷰받고 싶은 부분(선택)

Service 객체 로직 중심으로 리뷰 받고 싶어요.

논의하고 싶은 부분(선택)

BillAction이 Action를 상속하는 구조는 어떨까요?

---

질문 답변

1. presentation, application, domain, persistence 패키지로 나눠놓았는데, 다시 생각해보니 왜 repository가 domain과 같이 있지 않고, persistence로 분리해 놓았는지 의문이 든다.

repository는 domain 패키지에 있는 것이 더 자연스러워 보입니다.
repository를 domain의 일급컬렉션으로 생각하고 있기 때문입니다.

2. @PathVariable(name 명시할건지?) gradle, intellij 이슈있다함.

gradle로 빌드할거여서 상관없을 것 같긴한데 명시해도 좋다고 생각해요.

3. eventKey 조회하는 것을 컨트롤러에서 EventRepository 조회하도록 할건지?

이해가 잘 안되지만 컨트롤러에서 Repository에 바로 접근하는 것은 계층화를 깨서 안좋아보입니다.

🫡 참고사항

[kunsanglee]

서비스 로직 잘 작성해주신 것 같아요.
그 외에 코멘트가 필요한 부분 달아 놨습니다.
고생하셨어요 👍

[3Juhwan]

고생하셨습니다 :) 회의에서 논의할 사항을 코멘트에 남겨 놨습니다. 확인 부탁드려요

[kunsanglee]

의견이 궁금한 부분이 조금 있었고, 로직 관련하여 남길 리뷰는 없었던 것 같아요.
코멘트 확인해보시고 의견 남겨주세요.
고생 많으셨습니다 👍

[kunsanglee]

Suggested change

	List<BillAction> actions = billActionRepository.findByAction_Event(event)
	.stream()
	.sorted(Comparator.comparing(BillAction::getSequence).reversed())
	.limit(requests.size())
	.toList();
	assertThat(actions).extracting("title", "price")
	.containsExactly(
	tuple("인생맥주", 15_000L),
	tuple("뽕족", 10_000L)
	);
	List<BillAction> actions = billActionRepository.findByAction_Event(event);
	assertThat(actions).extracting("title", "price")
	.containsExactlyInAnyOrder(
	tuple("인생맥주", 15_000L),
	tuple("뽕족", 10_000L)
	);

containsExactly 대신 순서 상관없이 비교할 수 있는 containsExactlyInAnyOrder 를 사용하면
현재 테스트에서 하고있는 불필요한 정렬 부분을 제거할 수 있어요.

[Arachneee]

스택처럼 순차적으로 저장이 되었는지 확인하고 싶어서 정렬을 했습니다.
테스트코드에 로직이 있는 것이 불편하긴 하네요. 더 좋은 방법을 생각해봐야겠어요.

[khabh]

이상의 제안대로 정렬 로직을 제외하는 대신 다음과 같이 assert 부분을 sequence에 대한 검증도 가능하도록 수정했습니다~

assertThat(actions).extracting(BillAction::getTitle, BillAction::getPrice, BillAction::getSequence)
                .containsExactlyInAnyOrder(
                        tuple("뽕족", 10_000L, 1L),
                        tuple("인생맥주", 15_000L, 2L)
                );

[3Juhwan]

extracting 괜찮을까요? 깜박하고 언급하지 않았네요. 저는 테스트에 리플렉션을 지양하는 편입니다. 다만, 지금은 그대로 가도 괜찮다고 생각해요.

[Arachneee]

getter가 열려있는 상황이라 extracting을 굳이 쓸 필요는 없을 것 같아요.

[kunsanglee]

a: 도메인에서 null 검증을 하지않는 대신 presentation layer에서 BillAction을 생성 요청하는 DTO(BillActionSaveRequest)에서 @NotNull 애너테이션으로 검증했고 이미 모두와 어디에서 어떻게 값 검증을 할 것인지 논의 했었지만, 검증코드 작성 스타일 차이에서 오는 인지비용이 생각보다 더 들었습니다.

제가 테스트코드를 읽으며 생각한 과정을 말해볼게요.

1. 해당 테스트 코드의 @DisplayName을 보고 어떤 기능을 수행하고, 어떤 것을 검증하려는지 파악한다.
2. null과 빈 문자열에 대한 값을 검증하지 않는 것을 보고, 실수로 빠뜨린 건가? 생각한다.
3. DTO에서 @NotNull 애너테이션으로 검증한다고 말한 것을 떠올린다.
4. BillAction을 생성 요청하는 DTO의 이름은 모르나, 일단 presentation.request 패키지에서 찾아서 검증 애너테이션이 잘 붙어있는지 확인한다.

이런 과정을 통해 인지했고, 만약 클라이언트에서 요구하는 DTO에 대한 수정사항으로 인해 변경하다가 검증 애너테이션을 잘못 달거나, 빼먹는다면? 또는 DTO를 통해 생성하지 않고 객체를 생성할 시 실수를 인지하기 어려울 수 있겠다는 느낌을 받았습니다.

테스트 코드를 내가 작성하지 않은 기능에 대한 문서라고 생각한다면, 이런 부분이 조금 문제가 된다고 생각합니다. 이 부분에 대해 감자, 백호, 망쵸는 어떤가요? 의견이 궁금합니다.

[3Juhwan]

이런 과정을 통해 인지했고, 만약 클라이언트에서 요구하는 DTO에 대한 수정사항으로 인해 변경하다가 검증 애너테이션을 잘못 달거나, 빼먹는다면? 또는 DTO를 통해 생성하지 않고 객체를 생성할 시 실수를 인지하기 어려울 수 있겠다는 느낌을 받았습니다.

controller 테스트에서 해당 어노테이션에 대한 검증을 하면 해결할 수 있지 않을까요?

인지 비용이 든다는 것을 이해해요. 우리 모두 코드 작성 방식이 달라서, 누군가는 인지 비용이 클 수 있을 것 같아요. 코드 컨벤션을 정함에 있어서 기준이 모두의 인지 비용을 줄이는 방향이라는 생각이 드네요. 도메인 테스트에서는 이상이 큰 비용을 부담하는 것 같고요. 그 비용이 크다면, 저는 널 검증을 도메인에서 하고 비용을 줄이는 편을 택하겠어요 :)

[kunsanglee]

일전에 이미 논의한 주제라서 지금 변경이 필요하다고 하는건 아닙니다. 개인적인 인지 비용은 시간이 조금 지나면 익숙해져서 해결될 것 같습니다. 다만 혹시라도 앞서 언급한 예상되는 실수가 실제로 발생하면 그 때 가서 도메인에서의 검증을 재고해봐도 늦지 않을 것 같아요.

[kunsanglee]

컨트롤러 테스트시 MockMvc, ObjectMapper 객체를 거의 필수로 선언할텐데요, BillActionService 객체를 비롯한 테스트에 필요한 객체를 MockMvc, ObjectMapper와 같은 객체 위 또는 아래에 묶어서 선언하고 싶은데 어떤 순으로 정렬 하는게 좋을까요? 제 개인적인 의견은 MockMvc와 ObjectMapper는 getter처럼 밑에 몰아두는 것이 더 좋지 않을까 합니다.

[Arachneee]

상속구조를 활용해서 컨트롤러 테스트간의 중복된 객체를 제거하면 좋을 것 같아요. 상속구조를 활용하면 Spring 컨테이너를 클래스마다 새로 띄우지 않는 장점도 있습니다.

[3Juhwan]

이상의 의견대로 정렬하는 것이 좋다고 생각합니다. 정렬 기준은 뭐든 선택만 하면 될 것 같습니다.
백호 의견대로 중복되는 것은 별도의 클래스로 분리하면 좋을 것 같습니다. 테스트 클래스가 더 많아지면 그때 분리해 보는 건 어떨까요?

[kunsanglee]

망쵸 의견에 동의합니다. 제 생각에도 최소한 지금 진행하고 있는 이슈 두 건이 모두 머지된 이후에 테스트 상속을 고려하는 것이 적절할 것 같아요.

[3Juhwan]

리뷰 반영하느라 고생하셨습니다 😀
마지막으로 사소한 부분 코멘트 남겼으니 확인해 주세요 :)

추가로, 테스트 코드에 FE 크루의 이름을 넣는 것은 어떠한가요? BE 코드지만 우리 같은 팀원 이름이 모두 담기면 좋을 것 같아요 😍

[github-actions]

Test Results

16 tests   16 ✅  1s ⏱️
 6 suites   0 💤
 6 files     0 ❌

Results for commit e489d82.