Code Review 요청

[ss0ngcode]

Final 중간발표 이후 코드리뷰 요청 보내드립니다!

노션 페이지 : https://www.notion.so/Final_-_1-Omocha-eff998bfd0524dd3926822834f9cd433?pvs=4

[yoowonyoung]

Q1. 패키지 구조

패키지를 잘 나누기 위해서 가장 먼저 생각해야 할 것은, 어떤 컨셉으로 프로젝트를 구조화 할것인가~ 라고 생각해요. 현재 제가 프로젝트의 구조를 살펴 봣을땐 프로젝트 내부에서도 여러 컨셉이 혼재 되어있다는 느낌을 받고 있어요. 한가지 예시로 Entity/Dto/Domain/Request&Response 이렇게 다양한 종류의 클래스들이 존재 하는것으로 보이는데, 패키지 구조를 보면 omocha-domain/src/main/java/org/auction/domain/auction/domain/entity/AuctionEntity.java 와 같이 domain 안에 auction 안에 domain이 다시 있고, 그 안에 entity까지 있어요. 도메인과 인프라스트럭처를 구분하고 싶었던것 같은데 현 상태는 패키지의 depth만 깊어진 상태로 보여요. 또 bid쪽은 domain 패키지가 하위에 없고 바로 entity패키지가 있기도 하구요. 또 chat쪽은 domain 패키지 하위에 dto패키지가 있고...
이게 프로젝트의 정확한 컨셉이 정해지지 않아서 그런것으로 보여서요. 클래스를 나누는 기준도 동일합니다. 우리 프로젝트에서 어떤 아키텍처를 쓰고, 그중에서 어떤 클래스들을 쓸지(Dto 대신 Domain을 주고 받는 경우도 있어서 Dto를 생략 할수도 있어요) 와 같은 사항들을 먼저 정해야지 올바르게 구조화 할수 있을것이에요~

[yoowonyoung]

Q2. 낙찰 처리
스케줄러를 통해서 처리하는것도 나쁜 선택지는 아니지만, 매 분마다 스케줄러가 돌아야 하는 상황에서 스케줄링 한 싸이클이 1분 이상 소요되게 된다면 올바르게 처리되지 못할 가능성이 있습니다. 이런 경우엔 외부 시스템을 이용하는것도 방법이 될텐데요, 간단하게는 Redis를 사용 하는 방법도 있습니다. 경매가 등록될때 Redis에 해당 경매 Key를 등록하고 경매 시간 만큼 Key의 만료 시간을 설정 하는것이지요. 그 후, Redis의 키 만료 이벤트를 구독 하고 있다가 해당 키가 만료 될 때 처리를 하면 경매 건별로 처리를 할수가 있겠죠? 다만, Redis는 신뢰성을 보장하는 솔루션은 아니기 때문에, 신뢰성이 좀 더 중요하다면 Message Queue들의 지연 메시지 기능을 써보는것도 대안이 될 수 있겠네요~

[yoowonyoung]

Q3. Entity의 속성
저도 일단은 ImageEntity에서 모든 이미지를 한번에 확인할 수 있다는 장점 때문에 그쪽이 더 선호되는데요. 사실 현업에서는 연관관계를 강하게 맺어두지 않기도 합니다. 유지보수 때문에요. 연관관계를 강하게 맺어준다면 마이그레이션을 할때 상당히 골치가 아파지기 때문이죠. 그래서 어느정도 역정규화를 한 후에 데이터를 중복해서 저장시키는 경우도 많이 있고, 연관관계가 있더라도 실제 FK제약조건을 걸어두지는 않는 경우도 많습니다

[yoowonyoung]

Q4. QueryDSL
이 경우에는 Response가 아니라 두 테이블을 조인한 새로운 Domain이라고 보는편이 맞겟네요. 그렇다면 Domain에 위치 하는게 맞지 않을까요?

[yoowonyoung]

Q5. Service 코드 개선
먼저, 겹치는

		MemberEntity memberEntity = memberRepository.findById(memberId)
			.orElseThrow(() -> new MemberNotFoundException(MEMBER_NOT_FOUND));
			
		AuctionEntity auctionEntity = auctionRepository.findByIdWithImages(auctionId)
			.orElseThrow(() -> new AuctionNotFoundException(AUCTION_NOT_FOUND));

와 같은 부분들은 해당 서비스 코드 뿐만 아니라 다른 서비스코드 전역에 걸쳐서도 나타날것 같은데요. 이런 경우는 Repository 자체를 한번 더 래핑 하는게 어떨까요? RepositoryAdpater 혹은 CommandService 등을 만든 후, 반복되는 orElseThrow() ~ 를 해당 클래스에서 처리하고 바로 Domain등으로 변환해서 반환하는것이지요. 이렇게 한다면 저런 orElseThrow() ~ 를 없앨수 있습니다.
여기서 더 이어지는게 Entity를 Service에서 만드는게 아니다~ 라고 다른 멘토분이 말씀 해주셧던 부분인데, 아마 그 멘토님도 저런 Adapter클래스 혹은 Command 클래스등을 통해서 해당 클래스에서 Entity를 만들고, 비즈니스 로직이 있는 부분에서는 Domain 클래스로만 다루길 원하셔서 그렇게 말씀 하신것 같아요.
또, 너무 많은 코드가 존재한다고 하셨지만 사실 비즈니스 로직은 짧고 대부분이 저런 클래스 생성 로직인것으로 보이는데요, 그렇다면 그 클래스 생성 로직을 외부가 아닌 해당 클래스 내부에 두는것도 방법입니다.

return new AuctionListResponse(
				auction.getAuctionId(),
				auction.getTitle(),
				auction.getAuctionStatus(),
				auction.getStartPrice(),
				bidService.getCurrentHighestBidPrice(auction.getAuctionId()),
				concludeService.findConcludePrice(auction.getAuctionId()),
				bidService.findBidCount(auction),
				auction.getStartDate(),
				auction.getEndDate(),
				imageKeys
			);

이 코드에서, AuctionListResponse 의 생성자로 auction, imageKeys 등등을 받아서 생성할 수 있도록 만든다면,

return new AuctionListResponse(
				auction,
				bidService.getCurrentHighestBidPrice(auction.getAuctionId()),
				concludeService.findConcludePrice(auction.getAuctionId()),
				bidService.findBidCount(auction),
				imageKeys
			);

이런식으로 줄일수 있는거죠. 이것을 이제 생성자가 아니라 팩토리 메서드를 이용 한다면 인자가 여러개이기 때문에 of라고 명명한 후

return new AuctionListResponse.of(
				auction,
				bidService.getCurrentHighestBidPrice(auction.getAuctionId()),
				concludeService.findConcludePrice(auction.getAuctionId()),
				bidService.findBidCount(auction),
				imageKeys
			);

이렇게도 만들수가 있겠네요. 생성 로직은 팩토리 메서드 안에 두고요.

[yoowonyoung]

Q6. Entity를 넘겨주는 방법
두 질문에 대한 답변은 모두 안타깝게도 네 입니다. 물론 Entity를 직접 그대로 반환 한다면 편하죠. 하지만 불필요한 정보까지 같이 노출 되게 되고, 또 Entity가 통제를 벗어나 수정될 가능성도 생겨버리게 됩니다. 의도치 않은곳에서 Entity의 수정이 발생 되어버린다면 JPA의 Dirty Check에 걸려서 자동으로 저장이 되고, 그렇게 되면 의도치 않은 데이터 수정까지 발생되겟죠. 다만, Dto가 아닌 Domain으로 다루고, 내부 로직에서는 Domain을 이용해서 자유롭게 데이터를 주고 받다가, 최후 Controller에서 응답할때만 Response 클래스를 만드는 방식으로 접근 한다면 변환 과정을 어느정도는 줄일 수 있습니다~